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: rename bnToRlp to bnToUnpaddedBuffer #1293

Merged
merged 6 commits into from
Jun 11, 2021
Merged

util: rename bnToRlp to bnToUnpaddedBuffer #1293

merged 6 commits into from
Jun 11, 2021

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Jun 10, 2021

This PR renames bnToRlp to bnToUnpaddedBuffer to better describe its function (closes ethereumjs/ethereumjs-util#284)

It also converts some uses of unpadBuffer(toBuffer(bn)) to the function, and adds a test for it in the util package. bnToRlp is kept as a deprecated alias that can be removed in a future major version.

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #1293 (f9831c8) into master (26de933) will increase coverage by 1.95%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
block 86.44% <100.00%> (+0.24%) ⬆️
blockchain 84.36% <ø> (-0.07%) ⬇️
client 84.48% <ø> (ø)
common 88.25% <ø> (-0.46%) ⬇️
devp2p 83.82% <ø> (ø)
ethash 82.83% <ø> (ø)
tx 89.61% <100.00%> (+0.11%) ⬆️
vm 87.06% <ø> (+5.70%) ⬆️

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

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.

Thanks, nice, yes this is actually a lot more readable. 😄

Please always use @deprectated for the code docs on such deprecations, otherwise e.g. Visual Studio is not picking this up by striking through. Have added on a review-commit.

Unrelated to PR: I think these two/three methods bnToHex(), bnToUnpaddedBuffer() (and bnToRlp()) are very much misplaced in the types module and should rather go to bytes. Have added to the v6 notes (let me know if you disagree).

@holgerd77 holgerd77 merged commit 9a70b5a into master Jun 11, 2021
@holgerd77 holgerd77 deleted the rename-bnToRlp branch June 11, 2021 08:35
@ryanio
Copy link
Contributor Author

ryanio commented Jun 11, 2021

@holgerd77 sounds good, ah thanks for the @deprecated tag I forgot to add that.

I think we can go ahead and move those methods to the bytes file as a "tiny fixes" issue rather than breaking change, since they are exported the same on the top level ethereumjs-util module (i.e. import { bnToHex } from 'ethereumjs-util') so it wouldn't be breaking unless someone was importing from the file directly, but we don't suggest or encourage that anywhere.

@holgerd77
Copy link
Member

@ryanio hmm, I would prefer to rather be cautious here and only do on next breaking release, also absolutely no urgency here. In the README these modules are stated very explicitly and it is as valid to import from the respective modules (can be even seen as a bit more clean) than from the top level. So I guess this can't really be foreseen if people are doing this or not.

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.

bnToRlp: misleading name
2 participants