Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert fillBytes method to bigIntToBytes for lower golang version #3498

Merged
merged 4 commits into from
Jan 26, 2022

Conversation

ahangsu
Copy link
Contributor

@ahangsu ahangsu commented Jan 25, 2022

Summary

We are probably not going to higher golang version (1.15+) at this point, while fillBytes from big.Int is introduced in 1.15.
Changing all the fillBytes in ABI to self-made bigIntToBytes in favor of lower go version.

@ahangsu ahangsu self-assigned this Jan 25, 2022
jannotti
jannotti previously approved these changes Jan 25, 2022
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have preferred a fill-in method with the exact same signature and semantics as FillBytes, so the change would be even smaller, but I'mm totally fine with this.

Let's merge after checks pass.

@jannotti
Copy link
Contributor

I might have preferred a fill-in method with the exact same signature and semantics as FillBytes, so the change would be even smaller, but I'mm totally fine with this.

Let's merge after checks pass.

For what it's worth, emulating FillBytes exactly would be annoying, as it is willing to take a buffer of any size that's big enough. Our use case only needs to work on exactly the right size buffer.

jasonpaulos
jasonpaulos previously approved these changes Jan 25, 2022
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct. I agree with the earlier comments about copy and I left another, but they are minor

@ahangsu ahangsu dismissed stale reviews from jasonpaulos and jannotti via fa513ba January 25, 2022 17:41
@ahangsu
Copy link
Contributor Author

ahangsu commented Jan 25, 2022

I tried to emulate a method like FillBytes before, but we would need to introduce new type alias for big.Int (in favor of defining new methods), plus a bunch of type casting to use new method, and I am reluctant to create a larger delta.

The current fix should be sufficient for the purpose of casting big.Int to specific length of byte slice, please let me know your thoughts.

jasonpaulos
jasonpaulos previously approved these changes Jan 25, 2022
Co-authored-by: Will Winder <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2022

Codecov Report

Merging #3498 (17b964e) into master (2edd3de) will decrease coverage by 0.02%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3498      +/-   ##
==========================================
- Coverage   47.67%   47.64%   -0.03%     
==========================================
  Files         370      370              
  Lines       59832    59841       +9     
==========================================
- Hits        28522    28514       -8     
- Misses      28002    28011       +9     
- Partials     3308     3316       +8     
Impacted Files Coverage Δ
data/abi/abi_encode.go 64.08% <45.45%> (-0.82%) ⬇️
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
ledger/blockqueue.go 82.18% <0.00%> (-2.88%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
catchup/service.go 69.40% <0.00%> (-0.50%) ⬇️
network/wsNetwork.go 63.29% <0.00%> (+0.29%) ⬆️
network/wsPeer.go 68.88% <0.00%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2edd3de...17b964e. Read the comment docs.

@jannotti jannotti merged commit 762bc80 into algorand:master Jan 26, 2022
ahangsu added a commit that referenced this pull request Apr 1, 2022
jannotti pushed a commit that referenced this pull request Apr 2, 2022
…tation (#3856)

* Revert "Revert `fillBytes` method to `bigIntToBytes` for lower golang version (#3498)"


* leadingZeros update

* minor updates with elliptics

* byteLength
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants