-
Notifications
You must be signed in to change notification settings - Fork 68
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: added support for legacy pre EIP 155 transactions with no chainID (chainId=0x0) #2228
Conversation
Signed-off-by: Logan Nguyen <[email protected]>
…D=0x0 Signed-off-by: Logan Nguyen <[email protected]>
Signed-off-by: Logan Nguyen <[email protected]>
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
Additional check on chain id 1
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
packages/relay/src/lib/precheck.ts
Outdated
@@ -100,7 +100,7 @@ export class Precheck { | |||
chainId(tx: Transaction, requestId?: string) { | |||
const requestIdPrefix = formatRequestIdMessage(requestId); | |||
const txChainId = prepend0x(Number(tx.chainId).toString(16)); | |||
const passes = txChainId === this.chain; | |||
const passes = txChainId === '0x0' || txChainId === this.chain; |
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.
should we tighten this up a bit by checking to see if the v value is 27 or 28 (for legacy transactions) when the chain_id is zero?
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.
Yes this makes sense! Just pushed in an update for it
Signed-off-by: Logan Nguyen <[email protected]>
776c312
to
c589e4b
Compare
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
…D (chainId=0x0) (#2228) * feat: added support for legacy unprotected ETX pre EIP-155 (chainID=0x0) Signed-off-by: Logan Nguyen <[email protected]> * test: added new UT for precheck.chainId() with a case that has chainID=0x0 Signed-off-by: Logan Nguyen <[email protected]> * test: fixed UT to allow support for pre EIP155 txs Signed-off-by: Logan Nguyen <[email protected]> * feat: added isLegacyUnprotectedEtx() method Signed-off-by: Logan Nguyen <[email protected]> --------- Signed-off-by: Logan Nguyen <[email protected]>
#2240) fix: added support for legacy pre EIP 155 transactions with no chainID (chainId=0x0) (#2228) * feat: added support for legacy unprotected ETX pre EIP-155 (chainID=0x0) * test: added new UT for precheck.chainId() with a case that has chainID=0x0 * test: fixed UT to allow support for pre EIP155 txs * feat: added isLegacyUnprotectedEtx() method --------- Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: Logan Nguyen <[email protected]>
Description:
This PR introduces a check method,
isLegacyUnprotectedEtx()
, within theprecheck
class and utilized it inprecheck.chainId()
, allows for compatibility with pre EIP 155 transactions (chainId=0x0
, defaultv = {27, 28}
). Additionally, the PR added new UT coverage for the update and resolves any related UT issues.Related issue(s):
Fixes #2201
Notes for reviewer:
Checklist