Fix serious issue in .toString(16)
#295
Conversation
The hex encoding of some numbers is wrong. Since the string representation is also used for interoperability between BigInt libraries, this bug is causing a number modification when a BN.js number is converted into another BigInt class, for example the BigNumber of ethers.js.
|
FYI, I have confirmed this issue. @indutny Would you have an idea of when you would be able to get to this? I am debating including a hack to fix this temporarily into ethers if you don’t have time to get to it. :) |
|
@alexdupre thanks for the fix. Can you explain what's wrong with current code and how moving |
|
The issue is present when these conditions are true:
In this case the |
|
Make sense, thanks. |
|
If you are 100% sure that the fix is correct, then I think it should be released as |
|
I think changes are correct but it would be great if somebody except @ricmoo confirmed that everything is correct. |
|
FWIW I think this may be a good candidate for a property-based testing methodology (it'd be interesting to know if it would have spotted this issue as well). |
|
Any update? |
|
Thank you for the reminder! Published as |
|
@fanatid Could I bug you to also get the version in |
|
Only @indutny can do this :/ |
Fixes correctness bug for .toString(16): indutny/bn.js#295 ethers-io/ethers.js#3017
Fixes correctness bug: indutny/bn.js#295
Fixes correctness bug for .toString(16): indutny/bn.js#295 ethers-io/ethers.js#3017
|
Could this be backported to v4? |
|
Hey need help getting it out
Sent from Yahoo Mail for iPhone
On Friday, November 8, 2024, 7:38 AM, Nikita Skovoroda ***@***.***> wrote:
Could this be backported to v4?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you commented.Message ID: ***@***.***>
|
* Add test for `.toString(16)` * Fix serious issue in `.toString(16)` The hex encoding of some numbers is wrong. Since the string representation is also used for interoperability between BigInt libraries, this bug is causing a number modification when a BN.js number is converted into another BigInt class, for example the BigNumber of ethers.js.
|
published as 4.12.1, thanks @ChALkeR! |
|
Hey, so what are you trying to say? |
## **Description**
- Unpin and dedupe `ethereumjs-util` and `ethereumjs-abi` while staying
on legacy v6
- Replace `import { BN } from 'ethereumjs-util'` with importing `BN`
explicitly from either `bn.js@4` or `bn.js@5`
- Mixing the two is not safe and can cause breakage.
- [Some on why this is A Bad
Thing](ethereumjs/ethereumjs-util#250)
- We don't get complete and correct typings with importing the old
re-export
- This adds both v4 and v5 as explicit dependencies - ideally v4 will be
removed as transition to v5 concludes
- Add EIP-1191 address checksum algorithm support for
`toChecksumAddress()`
- Unblock relying on the ancient and deprecated `[email protected]`
- [`ethereumjs-util`
changelog](https://github.com/ethereumjs/ethereumjs-util/blob/master/CHANGELOG.md)
-
[`[email protected]`](ethereumjs/ethereumjs-util@v6.1.0...v6.2.0)
## **Related issues**
- indutny/bn.js#209
- indutny/bn.js#295
- #12010
#### Blocking
- #11932
- #12043
- #11968
## **Manual testing steps**
## **Screenshots/Recordings**
### **Before**
### **After**
## **Pre-merge author checklist**
- [x] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
## **Pre-merge reviewer checklist**
- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
---------
Co-authored-by: Nicolas MASSART <[email protected]>
Co-authored-by: Nico MASSART <[email protected]>
5 participants
In some circumstances the hex encoding of big numbers is wrong. In addition to a display issue, given that the the hex string if often used as an intermediate representation in transport/conversion scenarios, the re-constructed big number can actually change its value, creating serious issues.