hiero-ledger / hiero-ledger/hiero-sdk-cpp
[Intermediate]: Populate `EthereumTransactionDataEip1559::mRecoveryId` via `EthereumFlow`
- Dominant language
- C++
- Stars
- 42
- Forks
- 108
- Avg merge
- 11h 45m
- Merged PRs (30d)
- 2
Description
### 🧩 Intermediate Friendly
This issue is a good fit for contributors who are already familiar with the Hiero C++ SDK and feel comfortable navigating the codebase.
Intermediate Issues often involve:
- Exploring existing implementations
- Understanding how different components work together
- Making thoughtful changes that follow established patterns
The goal is to support deeper problem-solving while keeping the task clear, focused, and enjoyable to work on.
> [!IMPORTANT]
> ### 🧭 About Intermediate Issues
>
> Intermediate Issues are a great next step for contributors who enjoy digging into the codebase and reasoning about how things work.
>
> These issues often:
> - Involve multiple related files or components
> - Encourage investigation and understanding of existing behavior
> - Leave room for thoughtful implementation choices
> - Stay focused on a clearly defined goal
>
> Other kinds of contributions — from beginner-friendly tasks to large system-level changes — are just as valuable and use different labels.
### 👾 Description of the Task
PR #1595 added `ECDSAsecp256k1PrivateKey::getRecoveryId(...)` (the primitive that derives an Ethereum-style recovery ID `v` from a `(r, s)` signature and the original message). The primitive landed without any callers, so `EthereumTransactionDataEip1559::mRecoveryId` is still never populated and the SDK does not actually emit a properly-signed EIP-1559 transaction yet.
The smoking gun lives in the integration test: [`EthereumTransactionIntegrationTests.cc:116`](../../src/sdk/tests/integration/EthereumTransactionIntegrationTests.cc#L116) hardcodes the recovery ID as `"01"`, and the test is currently `DISABLED_` because that hardcode means the test cannot reliably round-trip through signature recovery. Replacing the hardcode with a real computed value is the concrete shape of this fix.
Issue [#684](https://github.com/hiero-ledger/hiero-sdk-cpp/issues/684) (closed by PR #1595) describes the broader goal as "proper Ethereum transaction construction" — adding the primitive was step one; this issue closes the loop by wiring it into the signing flow so the produced transaction has a usable recovery ID.
Relevant files:
```
src/sdk/main/include/ECDSAsecp256k1PrivateKey.h
src/sdk/main/src/ECDSAsecp256k1PrivateKey.cc
src/sdk/main/include/EthereumTransaction.h
src/sdk/main/src/EthereumTransaction.cc
src/sdk/main/include/EthereumTransactionDataEip1559.h
src/sdk/main/src/EthereumTransactionDataEip1559.cc
src/sdk/main/include/EthereumFlow.h (only if also wiring the legacy path — see Approach)
src/sdk/main/src/EthereumFlow.cc (only if also wiring the legacy path — see Approach)
src/sdk/tests/unit/EthereumTransactionDataEip1559UnitTests.cc
src/sdk/tests/integration/EthereumTransactionIntegrationTests.cc
```
Reference comment from PR #1595 review (maintainer): *"this PR adds the primitive, the next one closes the loop on #684's stated goal of 'proper Ethereum transaction construction.'"*
#### Prior art: closed PR #984
[PR #984](https://github.com/hiero-ledger/hiero-sdk-cpp/pull/984) (gsstoykov, opened 2025-07-09, **closed without merging** 2026-04-17) targeted issue [#912](https://github.com/hiero-ledger/hiero-sdk-cpp/issues/912) (Jumbo transactions / HIP-1086) but bundled a draft of exactly this wiring inside it. Read its diff before starting — even though it was never merged, it is the closest thing to a worked example that exists. Specifically reusable:
- The shape of computing the recovery ID after signing and replacing the hardcoded `"01"` at the integration test (its diff against `EthereumTransactionIntegrationTests.cc` is the integration-test pattern this issue should reproduce).
- The un-`DISABLED_`'ing of `SignerNonceChangedOnEthereumTransaction` once the recovery ID is computed for real.
Caveats — do **not** copy these from #984:
- PR #984 named its primitive `calculateRecoveryId(msgHash, r, s)`. PR #1595 later landed a *different* primitive `getRecoveryId(...)` with a different signature. Use the merged `getRecoveryId` API.
- PR #984 also marked `EthereumFlow` `[[deprecated]]`. That deprecation never landed; verify in [`EthereumFlow.h`](../../src/sdk/main/include/EthereumFlow.h) — the class is **not** marked deprecated in the v0.55.0 release commit. Don't add the deprecation as part of this PR; that's a separate conversation.
- PR #984's Jumbo-transaction work (the `mJumboSmartContractBytecode` helper, related test additions) belongs to issue #912 and is **out of scope here**.
### 💡 Proposed Approach
**Primary target: the `EthereumTransaction` direct path.** This is the unambiguous forward direction — Jumbo Transactions (HIP-1086) make `EthereumTransaction` viable end-to-end without `EthereumFlow`'s file-create-and-append dance, and PR #984 had already started moving in that direction by marking `EthereumFlow` deprecated. Wire `EthereumTransaction` first; treat `EthereumFlow` as a separate decision (see below).
When the EIP-1559 signing path inside `EthereumTransaction` (or the `EthereumTransactionDataEip1559` it dispatches to) signs the RLP-encoded message body with an `ECDSAsecp256k1PrivateKey`:
1. Compute the recovery ID by calling `getRecoveryId(...)` on the private key after signing. (Confirm the exact `getRecoveryId` signature in [`ECDSAsecp256k1PrivateKey.h`](../../src/sdk/main/include/ECDSAsecp256k1PrivateKey.h) — that is the merged primitive and the one to use.)
2. Store the result in `EthereumTransactionDataEip1559::mRecoveryId`.
3. Ensure the recovery ID is included when the transaction is serialized for submission, in the byte position required by EIP-1559 (the `yParity` field).
**`EthereumFlow` — separate decision:** `EthereumFlow` is **not** currently marked `[[deprecated]]` in main (verify in `EthereumFlow.h`). Three reasonable options for the contributor:
- **(a)** Wire only `EthereumTransaction` in this PR; leave `EthereumFlow` for a follow-up. Simplest scope. Recommended unless the `EthereumFlow` wiring is genuinely a one-line change.
- **(b)** Wire both paths in this PR. Acceptable if the diff stays small and `EthereumFlow` shares the same EIP-1559 signing helper as `EthereumTransaction`.
- **(c)** Land the `[[deprecated]]` attribute on `EthereumFlow` here as well. **Not recommended** — that is a public-API decision worth its own conversation rather than a side effect of a wiring fix.
Pick (a) by default; pick (b) only if you confirm during the work that the second wiring is trivial. Note your choice and rationale in the PR description.
The change should be localized to the EIP-1559 signing path and should not affect legacy (`EthereumTransactionDataLegacy`) or any other transaction type.
### 👩💻 Implementation Steps
- [ ] Read `getRecoveryId` in [src/sdk/main/include/ECDSAsecp256k1PrivateKey.h](../../src/sdk/main/include/ECDSAsecp256k1PrivateKey.h) / [.cc](../../src/sdk/main/src/ECDSAsecp256k1PrivateKey.cc) and write down its exact signature, input expectations, and return semantics (in particular: what does it return on failure, and is the message argument the raw payload or a hash?).
- [ ] Read closed [PR #984](https://github.com/hiero-ledger/hiero-sdk-cpp/pull/984)'s diff against `EthereumTransactionIntegrationTests.cc` to see the integration-test wiring shape. Use it as a reference for the structure (compute recid → replace hardcoded `"01"` → un-`DISABLED_`), but **do not** copy its `calculateRecoveryId` (that primitive never landed) or its `EthereumFlow` deprecation.
- [ ] Locate the EIP-1559 signing call site inside `EthereumTransaction` / `EthereumTransactionDataEip1559`. Trace through `Transaction::sign(...)` if needed.
- [ ] After each successful signature, call `getRecoveryId(...)` and store the result in `EthereumTransactionDataEip1559::mRecoveryId`. Decide on an error strategy when the primitive signals failure — recoverable failure should bubble up; do not silently emit a transaction with an undefined `v`.
- [ ] Update `EthereumTransactionDataEip1559::toBytes()` (or wherever the encoded transaction is produced) to include `mRecoveryId` in the `yParity` position per EIP-1559.
- [ ] Decide whether to also wire `EthereumFlow`'s signing path in this PR (see "**`EthereumFlow` — separate decision**" in the Approach). Default: skip; file a follow-up.
- [ ] Add a unit test in `EthereumTransactionDataEip1559UnitTests.cc` that:
- Signs a known message with a known key
- Asserts `mRecoveryId` is populated to a value in `{0, 1}` (or wider per the primitive's contract)
- Asserts the encoded bytes contain the expected `yParity`
- [ ] In [`EthereumTransactionIntegrationTests.cc`](../../src/sdk/tests/integration/EthereumTransactionIntegrationTests.cc), replace the hardcoded `recoveryId = HexConverter::hexToBytes("01")` (currently at line 116) with a real computed value via `getRecoveryId`. Once the test passes deterministically, drop the `DISABLED_` prefix from `SignerNonceChangedOnEthereumTransaction` (currently at line 38).
- [ ] Run the unit suite and the integration suite (against Solo) locally and confirm green.
### ✅ Acceptance Criteria
- [ ] `EthereumTransactionDataEip1559::mRecoveryId` is populated whenever the `EthereumTransaction` direct path signs an EIP-1559 transaction.
- [ ] Encoded transaction bytes include `yParity` in the EIP-1559 position.
- [ ] At least one unit test covers the populated value and at least one covers the encoded byte placement.
- [ ] `EthereumTransactionIntegrationTests::SignerNonceChangedOnEthereumTransaction` no longer hardcodes `recoveryId` to `"01"` and is no longer `DISABLED_`.
- [ ] If `EthereumFlow` was also wired in this PR, that decision and rationale are stated in the PR description; if not, a follow-up issue is filed for it.
- [ ] No behavior change to `EthereumTransactionDataLegacy` or any non-Ethereum transaction type.
- [ ] No `[[deprecated]]` attribute is added to `EthereumFlow` in this PR (that is a separate conversation).
- [ ] All existing unit and integration tests continue to pass.
---
### 📋 Step-by-Step Contribution Guide
To help keep contributions consistent and easy to review, we recommend following these steps:
- [ ] Comment `/assign` to request the issue
- [ ] Wait for assignment
- [ ] Fork the repository and create a branch
- [ ] Set up the project using the instructions in `README.md`
- [ ] Make the requested changes
- [ ] Sign each commit using `-s -S`
- [ ] Push your branch and open a pull request
Read [Workflow Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/workflow.md) for step-by-step workflow guidance.
Read [README.md](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/README.md) for setup instructions.
❗ Pull requests **cannot be merged** without `S` and `s` signed commits.
See the [Signing Guide](https://github.com/hiero-ledger/hiero-sdk-cpp/blob/main/docs/training/signing.md).
### 🤔 Additional Information
- EIP-1559 reference: https://eips.ethereum.org/EIPS/eip-1559 — the `yParity` field replaces the legacy `v` and must be `0` or `1`.
- Cross-SDK reference: the JS SDK (`hiero-sdk-js`) and the Java SDK both populate the recovery ID at signing time. Following their dispatch sequence is a reasonable mental model when deciding where in the C++ signing path to compute it.
- Related but distinct: issue [#912](https://github.com/hiero-ledger/hiero-sdk-cpp/issues/912) (Jumbo `EthereumTransaction` / HIP-1086) is still open and tracks the broader effort to make `EthereumTransaction` a complete replacement for `EthereumFlow`. This issue does **not** close that one — it just makes the EIP-1559 signing output usable along the way. Coordinate in the PR description if your work meaningfully overlaps with anyone actively picking up #912.
- If you discover that the existing signing surface does not give you access to the original message bytes after signing (so you can't pass them to `getRecoveryId`), call this out in the PR description — it may indicate a small internal API change is needed and a maintainer will weigh in.
If you have questions while working on this issue, feel free to ask! [Hiero-SDK-C++ Discord](https://discord.com/channels/905194001349627914/1337424839761465364)
Contributor guide
Research direction
Start with getRecoveryId in src/sdk/main/include/ECDSAsecp256k1PrivateKey.h and .cc, then trace the EIP-1559 signing path in EthereumTransaction and EthereumTransactionDataEip1559. Review EthereumTransactionDataEip1559UnitTests.cc and the disabled SignerNonceChangedOnEthereumTransaction test in EthereumTransactionIntegrationTests.cc. Done means mRecoveryId is populated, yParity is encoded, and the integration test computes its recovery ID and passes.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- blockchain, cryptography
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100