feat: fix eip1559 recovery id#1605#1649
Conversation
|
Hey @Adityarya11 👋 thanks for the PR! This comment updates automatically as you push changes -- think of it as your PR's live scoreboard! PR Checks✅ DCO Sign-off -- All commits have valid sign-offs. Nice work! ✅ GPG Signature -- All commits have verified GPG signatures. Locked and loaded! ✅ Merge Conflicts -- No merge conflicts detected. Smooth sailing! ✅ Issue Link -- Linked to #1605 (assigned to you). 🎉 All checks passed! Your PR is ready for review. Great job! |
github-actions
Bot
added
the
status: needs review
Adityarya11
force-pushed
the
feat/wire-eip1559-recovery-id#1605
branch
from
2d16128 to
aff9185
Compare
Signed-off-by: Aditya Arya <arya050411@gmail.com>
Signed-off-by: Aditya Arya <arya050411@gmail.com>
Signed-off-by: Aditya Arya <arya050411@gmail.com>
Signed-off-by: Aditya Arya <arya050411@gmail.com>
Adityarya11
force-pushed
the
feat/wire-eip1559-recovery-id#1605
branch
from
0aee1c0 to
f89cbfb
Compare
📝 WalkthroughWalkthroughAdds EIP-1559 signing: a new public EthereumTransaction::sign(...) method, EthereumTransactionDataEip1559::toSignBytes(), signing implementation that computes r/s and recovery id for EIP-1559, and integration/unit test updates to verify dynamic recovery-id and serialized placement. ChangesEIP-1559 Ethereum Transaction Signing
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sdk/main/src/EthereumTransaction.cc`:
- Around line 91-100: The code currently only checks for negative recovery IDs
from ecdsaKey->getRecoveryId but allows values >1 to be serialized into EIP-1559
yParity; update the check after const int recId = ecdsaKey->getRecoveryId(r, s,
messageToSign) to ensure recId is exactly 0 or 1 (throw std::runtime_error for
any other value), then assign eip1559Data->mRecoveryId using the validated recId
(static_cast<std::byte>(recId)) before calling mEthereumData =
eip1559Data->toBytes(); this ensures mRecoveryId holds a valid yParity and
prevents invalid serialization.
In `@src/sdk/main/src/EthereumTransactionDataEip1559.cc`:
- Around line 103-105: The EIP-1559 encoding paths currently hardcode an empty
access list (the RLPItem pushBack and the concatenation with 0x02), so
transactions with a non-empty mAccessList are encoded/hashed incorrectly; update
both toSignBytes() and toBytes() in EthereumTransactionDataEip1559 (replace the
placeholder empty access list RLPItem and the hardcoded concatenation with logic
that RLP-encodes mAccessList and appends it to the RLP list before returning the
concatenated 0x02 payload) — ensure you use the existing mAccessList member and
the same RLP encoding helpers used elsewhere to produce the correct access list
encoding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f339ba5-cbb6-420f-b076-36fccf7464a1
📒 Files selected for processing (6)
src/sdk/main/include/EthereumTransaction.hsrc/sdk/main/include/EthereumTransactionDataEip1559.hsrc/sdk/main/src/EthereumTransaction.ccsrc/sdk/main/src/EthereumTransactionDataEip1559.ccsrc/sdk/tests/integration/EthereumTransactionIntegrationTests.ccsrc/sdk/tests/unit/EthereumTransactionDataEip1559UnitTests.cc
Signed-off-by: Aditya Arya <arya050411@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sdk/main/src/EthereumTransaction.cc (1)
56-64:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinalize the EIP-1559 payload before calling the base
sign().
Transaction<EthereumTransaction>::sign(key)runs before parsingmEthereumData, generating the ECDSA signature, and validatingrecId. If anything throws on Lines 72-95,sign()exits with*thisalready mutated by the base signing path even though Line 100 never writes the finalized EIP-1559 bytes.Also applies to: 72-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdk/main/src/EthereumTransaction.cc` around lines 56 - 64, The current EthereumTransaction::sign(const std::shared_ptr<PrivateKey>&) calls Transaction<EthereumTransaction>::sign(key) before finalizing the EIP-1559 payload, which lets the base sign mutate *this* if subsequent EIP-1559 parsing/signature/recId validation (mEthereumData handling between lines ~72-100) throws; fix by moving/performing the EIP-1559 finalization (parse mEthereumData, compute ECDSA signature bytes, validate recId and produce the finalized EIP-1559 payload bytes) prior to invoking Transaction<EthereumTransaction>::sign(key), or alternatively defer any mutation in the base path until after finalization so that EthereumTransaction::sign only calls Transaction<...>::sign once the EIP-1559 bytes are finalized and safe to attach to *this*.
♻️ Duplicate comments (1)
src/sdk/main/src/EthereumTransactionDataEip1559.cc (1)
92-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
toBytes()consistent with the new signing preimage.
toSignBytes()now includesmAccessList, buttoBytes()still hardcodes an empty list at Line 83. That means any transaction with a non-empty access list is signed as one payload and serialized/submitted as another onceEthereumTransaction::sign()rebuildsmEthereumData.Suggested fix
std::vector<std::byte> EthereumTransactionDataEip1559::toBytes() const { RLPItem list(RLPItem::RLPType::LIST_TYPE); list.pushBack(mChainId); list.pushBack(mNonce); list.pushBack(mMaxPriorityGas); list.pushBack(mMaxGas); list.pushBack(mGasLimit); list.pushBack(mTo); list.pushBack(mValue); list.pushBack(mCallData); - list.pushBack(RLPItem()); + RLPItem accessListItem(mAccessList); + accessListItem.setType(RLPItem::RLPType::LIST_TYPE); + list.pushBack(accessListItem); list.pushBack(mRecoveryId); list.pushBack(mR); list.pushBack(mS); return internal::Utilities::concatenateVectors({ { std::byte(0x02) }, list.write() }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sdk/main/src/EthereumTransactionDataEip1559.cc` around lines 92 - 107, The toBytes() serialization must match toSignBytes() by including the actual access list instead of the hardcoded empty list; update the EthereumTransactionDataEip1559::toBytes() implementation to push mAccessList (or an RLP LIST_TYPE-wrapped mAccessList like in toSignBytes()) into the RLP list so serialized payload equals the signing preimage used by EthereumTransaction::sign(), and ensure any RLPItem construction and setType calls mirror the pattern used in toSignBytes() for mAccessList to avoid mismatched signatures when rebuilding mEthereumData.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/sdk/main/src/EthereumTransaction.cc`:
- Around line 56-64: The current EthereumTransaction::sign(const
std::shared_ptr<PrivateKey>&) calls Transaction<EthereumTransaction>::sign(key)
before finalizing the EIP-1559 payload, which lets the base sign mutate *this*
if subsequent EIP-1559 parsing/signature/recId validation (mEthereumData
handling between lines ~72-100) throws; fix by moving/performing the EIP-1559
finalization (parse mEthereumData, compute ECDSA signature bytes, validate recId
and produce the finalized EIP-1559 payload bytes) prior to invoking
Transaction<EthereumTransaction>::sign(key), or alternatively defer any mutation
in the base path until after finalization so that EthereumTransaction::sign only
calls Transaction<...>::sign once the EIP-1559 bytes are finalized and safe to
attach to *this*.
---
Duplicate comments:
In `@src/sdk/main/src/EthereumTransactionDataEip1559.cc`:
- Around line 92-107: The toBytes() serialization must match toSignBytes() by
including the actual access list instead of the hardcoded empty list; update the
EthereumTransactionDataEip1559::toBytes() implementation to push mAccessList (or
an RLP LIST_TYPE-wrapped mAccessList like in toSignBytes()) into the RLP list so
serialized payload equals the signing preimage used by
EthereumTransaction::sign(), and ensure any RLPItem construction and setType
calls mirror the pattern used in toSignBytes() for mAccessList to avoid
mismatched signatures when rebuilding mEthereumData.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8eb0d080-46b1-4394-8165-6b21ec0bd600
📒 Files selected for processing (2)
src/sdk/main/src/EthereumTransaction.ccsrc/sdk/main/src/EthereumTransactionDataEip1559.cc
|
Hi @rwalworth, |
1 participant
Description
This PR wires EIP-1559 signing in the SDK so the recovery ID is computed and serialized correctly for Ethereum transactions.
Changes
EthereumTransaction::sign()to computer,s, and recovery ID, and update the encoded transaction.EthereumTransactionDataEip1559::toSignBytes()to build the signing preimage.01).yParity/r/splacement in the serialized bytes.Related issue(s)
Fixes #1605
Notes for reviewer
EthereumTransactiondirect path.Follow-up issue: Wire EthereumFlow EIP-1559 signing to populate recoveryId #1660
Checklist
Summary by CodeRabbit
New Features
Tests