Util, Monorepo: hexStringToBytes -> prefixedHexStringToBytes #2830
Conversation
| * @param hex | ||
| * @returns | ||
| */ | ||
| function _nonPrefixedHexStringToBytes(hex: string) { |
There was a problem hiding this comment.
Alternative: move this to util but shield it so no one uses it
|
Ok, have renamed all, tests should be fixed, however, I am totally not sure regarding client, since we have either hex-prefixed strings or non-hex-prefixed strings there, so we should take a thorough look there (or: alternatively, temporarily use an internal |
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Minor question, but since we're making that change, would it be better to name this: |
I'm not sure what you are referring to here? The PR is open for review now, just tested locally that client works (you might have to use a custom datadir or wipe your datadir) |
Oh sorry, I thought you had implemented explicit naming for prefixed and unprefixed hex strings, had only looked at the description and not the code. Will review later! |
| @@ -10,7 +10,7 @@ Note: All paths are relative paths to the `VM` package root. | |||
|
|
|||
| 1. If you change the port number in `transition-cluster.ts` to anything other than 3000 (or run `transition-cluster` on a separate machine or different IP address from retesteth), update `test/vm/retesteth/clients/ethereumjs/start.sh` to reflect the right IP and port. | |||
|
|
|||
| 2. From VM package root directory, run `npx ts-node test/retesteth/transition-cluster.ts` | |||
| 2. From VM package root directory, run `npx ts-node test/retesteth/transition-cluster.cts` | |||
There was a problem hiding this comment.
This is not relevant for this PR, but added it here, since it felt silly to open another PR for this.
There was a problem hiding this comment.
I would really prefer if we do not introduce an extra type UnprefixedHexString and so to not encourage/diffuse people to go into a wild mix again.
If we have - for whatever reason - this 1-2 occasions where it is necessary that we use ourselves we should rather keep this local.
|
Otherwise this looks really great! 🤩 Wasn't aware that this has gotten so extensive! |
|
Alright, I will remove this And regarding the amount of changes: yes this was much more work than I anticipated 😅 I thought I could just "find-and-replace" but that was not the case 😂 |
|
Ok, removed (I thought I had put it in much more places 😂 ) |
| @@ -50,7 +52,7 @@ describe('EIP4895 tests', () => { | |||
| () => { | |||
| BlockHeader.fromHeaderData( | |||
| { | |||
| withdrawalsRoot: hexStringToBytes('00'.repeat(32)), | |||
| withdrawalsRoot: prefixedHexStringToBytes('0x' + '00'.repeat(32)), | |||
There was a problem hiding this comment.
Could use the zeros util
There was a problem hiding this comment.
(For this and other ones in the file)
| @@ -141,8 +141,8 @@ describe('simple merkle range proofs generation and verification', () => { | |||
| } | |||
|
|
|||
| // Special case, two edge proofs for two edge key. | |||
| const startKey = hexStringToBytes('00'.repeat(32)) | |||
| const endKey = hexStringToBytes('ff'.repeat(32)) | |||
| const startKey = prefixedHexStringToBytes('0x' + '00'.repeat(32)) | |||
There was a problem hiding this comment.
I think in this case it is cleaner to keep it like this, since endKey uses prefixedHexStringToBytes and startKey would now use zeros - makes it slightly harder to read (I do agree with block tests though these should+will be changed in 25f29dc)
|
One test failing, looking into it. |
Ah nvm, just noticed it is also failing on master. |
jochem-brouwer
dismissed stale reviews from gabrocheleau and holgerd77
via
25f29dc
holgerd77
changed the title
Closes #2827
Note: if we have methods which return hex strings, the return type should be either
PrefixedHexStringorNotPrefixedHexString. Rewriting this in all libraries was fine, except for client, since I am not sure if the return values of the methods are prefixed or not, which is very error-prone.TODO: