Address review from #10: unrepresentable chain mismatch, serde/derive - #11
Conversation
…quests The `chain` field was removed from `SigningRequest` and `AttachRequest` because it duplicated information already present in the `TransactionSpec` variant, creating a state where the two could disagree. The chain is now read from the transaction spec itself via a new `TransactionSpec::chain()` method, making the disagreement unrepresentable. The `UnsupportedChain` error variant was removed along with the `mismatched` function, replaced by a simpler `unknown_kind` refusal for unrecognized transaction shapes. The Bitcoin transaction spec field `fee_rate_sat_vb` was renamed to `fee_sat` to reflect that the fee is absolute rather than rate-based. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
📝 WalkthroughWalkthroughThe wire contract removes request-level chain fields. ChangesTransaction request flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SigningRequest
participant WalletService
participant TransactionSpec
Client->>SigningRequest: submit transaction specification
SigningRequest->>WalletService: provide signing or attachment request
WalletService->>TransactionSpec: match transaction variant
TransactionSpec-->>WalletService: select chain-specific path
WalletService-->>Client: return signed transaction or validation failure
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tinywallet-module/tests/module_e2e.rs (1)
259-287: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the transaction-ID mismatch branch in the focused service test.
verify_transferchecksexpected_txidbefore recipient validation, so the current decodable fixture reaches that branch. MatchFailure::InvalidInputand assert the message contains"txID does not match sha256(raw_data)"instead of checking only"InvalidInput". A valid protobuf fixture is not required becauseverify_transferdoes not parse protobuf.🤖 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 `@crates/tinywallet-module/tests/module_e2e.rs` around lines 259 - 287, Update refuses_a_request_the_module_cannot_build to assert the underlying Failure::InvalidInput result and verify its message contains "txID does not match sha256(raw_data)". Keep the existing wire-name assertion if appropriate, but replace the insufficient generic error check with the focused transaction-ID mismatch assertion.
🤖 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 `@crates/tinywallet-module/src/service/mod.rs`:
- Around line 108-109: Update the comment above build_unsigned to remove the
stale chain_of reference and state that the function matches TransactionSpec
directly, preserving the explanation that unrecognised shapes are rejected
before selecting a variant.
---
Outside diff comments:
In `@crates/tinywallet-module/tests/module_e2e.rs`:
- Around line 259-287: Update refuses_a_request_the_module_cannot_build to
assert the underlying Failure::InvalidInput result and verify its message
contains "txID does not match sha256(raw_data)". Keep the existing wire-name
assertion if appropriate, but replace the insufficient generic error check with
the focused transaction-ID mismatch assertion.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 602b74ae-4813-442e-aa42-6405b64b2c00
📒 Files selected for processing (7)
.github/workflows/ci.ymlCargo.tomlcrates/tinywallet-module/src/service/mod.rscrates/tinywallet-module/src/service/test.rscrates/tinywallet-module/tests/module_e2e.rssrc/wire/mod.rssrc/wire/test.rs
| // `chain_of` runs first so an unrecognised shape is refused before any | ||
| // arm is tried; the chain itself then comes from the variant. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale chain_of reference.
Line 108 says that chain_of runs first. build_unsigned no longer calls chain_of. State that the function matches TransactionSpec directly.
Proposed fix
- // `chain_of` runs first so an unrecognised shape is refused before any
- // arm is tried; the chain itself then comes from the variant.
+ // Match the transaction variant directly. Unknown variants are refused.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // `chain_of` runs first so an unrecognised shape is refused before any | |
| // arm is tried; the chain itself then comes from the variant. | |
| // Match the transaction variant directly. Unknown variants are refused. |
🤖 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 `@crates/tinywallet-module/src/service/mod.rs` around lines 108 - 109, Update
the comment above build_unsigned to remove the stale chain_of reference and
state that the function matches TransactionSpec directly, preserving the
explanation that unrecognised shapes are rejected before selecting a variant.
1 participant
Follow-up to #10, which merged before these landed. CodeRabbit raised two findings on it, both correct; this is the fix for each.
A request could name one chain and carry another 🟠
SigningRequestandAttachRequestdeserializedchainandtransactionindependently, so a mismatched pair was accepted and had to be caught at runtime.The suggestion was to reject mismatches during validation, or drop the duplicate field. I took the second — validation leaves a state that has to be detected, and detection is what a future code path forgets.
TransactionSpec::chain()is now the single source of truth and neither request carrieschain. The disagreement is unrepresentable rather than rejected.This deleted code rather than adding it: the module's
mismatched()helper, itsUnsupportedChainfailure variant, and that variant's wire error name existed only to catch this.chain()is infallible, which is worth explaining sinceTransactionSpecis#[non_exhaustive]. That attribute binds only downstream crates, and a downstream crate calls the method rather than matching the enum — so there is no wildcard to write, and adding a variant becomes a compile error in the file that has to handle it. My first attempt returnedResultwith anUnknownChainerror; the coverage gate caught the wildcard as permanently unreachable, which was the right signal that theResultwas unnecessary.The rejection tests you asked for could not be written — the state no longer compiles. Instead:
every_transaction_names_its_own_chainpins the variant→chain mapping for all four.txIDdisagrees with the claimed one — a real compromised-node scenario — asserting theInvalidInputwire name.serdefeature missingserde/derive🟡Correct, and invisible locally:
cargo check --features serdepassed here, because this crate's dev-dependency onserdewithderiveunified the feature into the lib build. A consumer takingtinywalletwithserdealone gets no derive and fails to compileChain.I confirmed the mechanism with
cargo treerather than just applying the patch, and the Cargo comment records it so the apparent redundancy is not "cleaned up" later.wirenow depends on plainserde, since the derive comes from the one place that owns it.Also here
fee_rate_sat_vb→fee_sat. It always carried an absolute fee, not a rate; the module was already mapping it toTransfer::fee. Caught while wiring up the host, before anything depended on the wrong name./vendor/— and this repo is itself vendored inside other checkouts, so it silently excluded the crate's own sources when built from one. It only did the right thing on CI by accident of the runner's path.Verification
274 tests pass, clippy clean workspace-wide at the strict bar,
wire/mod.rsat 100% line coverage, and the real-module E2E still signs EVM, multi-input Bitcoin and Solana byte-identically to the library.Summary by CodeRabbit
New Features
Bug Fixes