-
Notifications
You must be signed in to change notification settings - Fork 75
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
fix: removed chainId field for legacy EIP155 transactions (#2468) #2489
fix: removed chainId field for legacy EIP155 transactions (#2468) #2489
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Nice acceptance tests but there's a missing unit test.
packages/relay/src/formatters.ts
Outdated
@@ -147,7 +147,7 @@ const formatContractResult = (cr: any) => { | |||
const commonFields = { | |||
blockHash: toHash32(cr.block_hash), | |||
blockNumber: nullableNumberTo0x(cr.block_number), | |||
chainId: cr.chain_id, | |||
chainId: cr.chain_id === '0x' ? undefined : cr.chain_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing UT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
c7ebc0e
to
3be64cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG. One nit on comment
packages/relay/src/formatters.ts
Outdated
@@ -147,7 +147,7 @@ const formatContractResult = (cr: any) => { | |||
const commonFields = { | |||
blockHash: toHash32(cr.block_hash), | |||
blockNumber: nullableNumberTo0x(cr.block_number), | |||
chainId: cr.chain_id, | |||
chainId: cr.chain_id === EMPTY_HEX ? undefined : cr.chain_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a comment on what this does and why so other devs understand
Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Quality Gate failedFailed conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
) * fix: removed chainId field for legacy EIP155 transactions Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * test: added UT Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> * chore: added explanation Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com> --------- Signed-off-by: Logan Nguyen <logan.nguyen@swirldslabs.com>
Description:
This PR simply remove the chainId field if the transaction result is a legacy EIP155 transaction whose chainId equals to "0x".
Related issue(s):
Fixes #2468
Notes for reviewer:
Checklist