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

util: move misplaced functions #1825

Merged
merged 4 commits into from
Apr 4, 2022
Merged

util: move misplaced functions #1825

merged 4 commits into from
Apr 4, 2022

Conversation

acolytec3
Copy link
Contributor

@acolytec3 acolytec3 commented Mar 29, 2022

Addresses the below item from #1717

  • Move misplaced bigIntToHex() and bigIntToUnpaddedBuffer() methods (and tests) from types to the bytes module
  • Unify const stripZeros = function (a: any): Buffer | number[] | string and bigIntToUnpaddedBuffer() to const stripZeros = function (a: any): BufferLike (which also encompassed BN)? (from @holgerd77, thought about this while reading over https://github.com/ethereumjs/ethereumjs-util/issues/141 ) (Update @holgerd77: removed from TODO list)

On the second item, in looking at the code, I'm not really sure it makes sense to merge stripZeroes and bigIntToUnpaddedBuffer as they do different things. To my way of thinking, it makes much more sense to merge:

  • stripZeros
  • unpadBuffer
  • unpadArray
  • unpadHexString

@holgerd77 Any thoughts here since it looks like the second item was your recommendation?

@acolytec3
Copy link
Contributor Author

One note, I also modified unpadHexString to actually return a hex prefixed string. I guess this has been otherwise forever but it doesn't make sense to me that we would not return a hex prefixed string if the sole point of the function is to strip leading zeros. This can be reverted if the team disagrees.

@codecov
Copy link

codecov bot commented Mar 29, 2022

Codecov Report

Merging #1825 (3240172) into develop (9341b6a) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 85.17% <ø> (ø)
blockchain 83.84% <ø> (ø)
client 77.22% <ø> (ø)
common 94.94% <ø> (ø)
devp2p 82.69% <ø> (-0.14%) ⬇️
ethash 90.71% <ø> (ø)
trie 80.27% <ø> (ø)
tx 92.48% <ø> (ø)
util 89.43% <100.00%> (-0.02%) ⬇️
vm 82.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member

One note, I also modified unpadHexString to actually return a hex prefixed string. I guess this has been otherwise forever but it doesn't make sense to me that we would not return a hex prefixed string if the sole point of the function is to strip leading zeros. This can be reverted if the team disagrees.

Yes, I think that makes very much sense, actually thought so too some time ago.

First TODO item implementation also looks good.

@holgerd77
Copy link
Member

On the second item, in looking at the code, I'm not really sure it makes sense to merge stripZeroes and bigIntToUnpaddedBuffer as they do different things. To my way of thinking, it makes much more sense to merge

TBH, I can't really follow up which arguments I had here, especially since stripZeroes is actually an internal function, hmm. 🤔

I wonder if we want to do any further consolidation here, will cautiously remove this TODO item from the v6 list. We might give things like that some additional thought once we do a final clean-up on the the Util library (which is still needed with all the BigInt changes and stuff, we need to have some look here with a broader perspective and see if things are still consistent).

@holgerd77 holgerd77 force-pushed the move-misplaced-functions branch from 74de550 to 9f250cb Compare April 4, 2022 10:27
@holgerd77
Copy link
Member

Have rebased this, also added a note on the second TODO item on the initial comment. Would merge in this current state if tests pass.

@holgerd77 holgerd77 force-pushed the move-misplaced-functions branch from 9f250cb to 3240172 Compare April 4, 2022 11:06
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

LGTM

@holgerd77 holgerd77 merged commit 95f4779 into develop Apr 4, 2022
@holgerd77 holgerd77 deleted the move-misplaced-functions branch April 4, 2022 11:46
holgerd77 pushed a commit that referenced this pull request Apr 5, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
holgerd77 pushed a commit that referenced this pull request Apr 7, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
@holgerd77 holgerd77 mentioned this pull request Apr 7, 2022
51 tasks
g11tech pushed a commit that referenced this pull request Apr 29, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
holgerd77 pushed a commit that referenced this pull request May 4, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
g11tech pushed a commit that referenced this pull request Jun 2, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
g11tech pushed a commit that referenced this pull request Jun 3, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
holgerd77 pushed a commit that referenced this pull request Jun 8, 2022
* util: move misplaced functions

* add unpadBuffer back

* Fix tttttttttttttypo

* make unpadHexString return Hex String!
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.

2 participants