Skip to content

Address review from #10: unrepresentable chain mismatch, serde/derive #11

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.

Sign up for GitHub

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

Merged
senamakel merged 1 commit into from
Aug 11, 2026
Merged

Address review from #10: unrepresentable chain mismatch, serde/derive #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,17 @@ jobs:
report="target/coverage.json"
cargo llvm-cov --all-features --all-targets --json --output-path "$report"

below_threshold="$(jq -r '
# Anchored at the workspace root: this repo is vendored inside other
# checkouts, so an unanchored "/vendor/" match would exclude the
# crate's own sources when it is built from one of those paths.
below_threshold="$(jq -r --arg root "$PWD" '
.data[0].files[]
| select(.filename | contains("/src/"))
# Vendored submodules carry code from another repository, whose
# coverage is not this one to enforce. They only entered the report
# when the module crate made tinybus part of the build graph.
# (No apostrophes here: the whole jq program is single-quoted.)
| select(.filename | contains("/vendor/") | not)
| select(.filename | startswith($root + "/vendor/") | not)
| select(.summary.lines.percent < 90)
| "\(.filename): \(.summary.lines.percent)%"
' "$report")"
Expand Down
8 changes: 6 additions & 2 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,11 @@ btc = ["dep:bs58", "dep:bech32", "dep:ripemd", "dep:sha2"]
# Serde derives on the crate's core types (today `Chain`). A named gate rather
# than relying on `dep:serde` being implied, because `#[cfg(feature = "serde")]`
# on the enum has to name something that actually exists.
serde = ["dep:serde"]
# `serde/derive` is required, not incidental: `Chain` uses the derive macros.
# A local `cargo check --features serde` passes without it only because the
# dev-dependency on `serde` with `derive` unifies the feature in — a downstream
# consumer taking this crate with `serde` alone gets no derive and fails.
serde = ["dep:serde", "serde/derive"]
# EVM (Ethereum and compatible chains) address validation. Dependency-free:
# an EVM address is 20 hex-encoded bytes, so the whole format is spelled out
# in `src/address/evm.rs` rather than pulling in a chain client for it.
Expand Down Expand Up @@ -150,7 +154,7 @@ x402 = ["dep:serde", "dep:serde_json", "serde/derive", "eip712"]
# transaction building into a loadable module can take this crate with
# `default-features = false`, share one definition of the contract, and link no
# chain library at all. Same carve-out `tinydocs::spec` makes.
wire = ["serde", "serde/derive"]
wire = ["serde"]
# Transaction building and signing (`tinywallet::tx`). This is the only gate
# that pulls the `bitcoin` crate, for PSBT construction and secp256k1
# recoverable signing. Everything else — addresses, derivation, reference data
Expand Down
129 changes: 50 additions & 79 deletions crates/tinywallet-module/src/service/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@
//! into a rewrite loop over something that was already correct.

use tinybus::{Connection, Error as BusError, Result as BusResult};
use tinywallet::tx;
use tinywallet::wire::{
AttachRequest, Scheme, Signature, SignedTransaction, SigningPayload, SigningRequest,
TransactionSpec, UnsignedTransaction,
};
use tinywallet::{Chain, tx};

/// Well-known name and interface exported by the `TinyWallet` module.
pub const BUS_NAME: &str = "ai.tinyhumans.tinywallet.Wallet";
Expand All @@ -54,8 +54,10 @@ pub const OBJECT_PATH: &str = "/ai/tinyhumans/tinywallet/Wallet";
const INVALID_INPUT_ERROR: &str = "ai.tinyhumans.tinywallet.Error.InvalidInput";
/// Building or assembling the transaction failed. A caller cannot fix it.
const BUILD_FAILED_ERROR: &str = "ai.tinyhumans.tinywallet.Error.BuildFailed";
/// The chain named is not compiled into this module.
const UNSUPPORTED_CHAIN_ERROR: &str = "ai.tinyhumans.tinywallet.Error.UnsupportedChain";
// There is no `UnsupportedChain` error name. Every chain this build can name
// is compiled in, and a chain it cannot name arrives as a `TransactionSpec`
// variant it does not recognise — which is `InvalidInput`, because the request
// is one this module cannot act on rather than a capability that is missing.

/// The served object. Holds nothing: every call is self-contained.
struct Wallet;
Expand Down Expand Up @@ -87,19 +89,13 @@ enum Failure {
InvalidInput(String),
/// Building or assembling failed for a reason the caller did not cause.
BuildFailed(String),
/// The chain is not in this build.
UnsupportedChain(Chain),
}

/// Map a failure onto the wire name a host matches on.
fn into_bus_error(failure: Failure) -> BusError {
let (name, message) = match failure {
Failure::InvalidInput(message) => (INVALID_INPUT_ERROR, message),
Failure::BuildFailed(message) => (BUILD_FAILED_ERROR, message),
Failure::UnsupportedChain(chain) => (
UNSUPPORTED_CHAIN_ERROR,
format!("this build has no support for {chain}"),
),
};
BusError::MethodFailed {
name: name.to_string(),
Expand All @@ -109,32 +105,31 @@ fn into_bus_error(failure: Failure) -> BusError {

/// Compute the signing payloads for `request`.
fn build_unsigned(request: &SigningRequest) -> Result<UnsignedTransaction, Failure> {
let payloads = match (&request.transaction, request.chain) {
(
TransactionSpec::Btc {
from,
to,
amount_sat,
fee_rate_sat_vb,
utxos,
},
Chain::Btc,
) => {
let transfer = btc_transfer(from, to, *amount_sat, *fee_rate_sat_vb);
// `chain_of` runs first so an unrecognised shape is refused before any
// arm is tried; the chain itself then comes from the variant.
Comment on lines +108 to +109

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
// `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.

let payloads = match &request.transaction {
TransactionSpec::Btc {
from,
to,
amount_sat,
fee_sat,
utxos,
} => {
let transfer = btc_transfer(from, to, *amount_sat, *fee_sat);
let public = compressed_public_key(&request.public_key.key_hex)?;
let (_, digests) = transfer
.sighashes(&btc_utxos(utxos), &public)
.map_err(|e| build_failed(&e))?;
digests.into_iter().map(secp256k1_payload).collect()
}
(spec @ TransactionSpec::Evm { .. }, Chain::Evm) => {
spec @ TransactionSpec::Evm { .. } => {
vec![secp256k1_payload(
evm_transaction(spec)?
.digest()
.map_err(|e| build_failed(&e))?,
)]
}
(spec @ TransactionSpec::Solana { .. }, Chain::Solana) => {
spec @ TransactionSpec::Solana { .. } => {
// ed25519 signs the message itself — there is nothing to pre-hash,
// so this payload is the whole serialized message, not a digest.
vec![SigningPayload {
Expand All @@ -144,14 +139,11 @@ fn build_unsigned(request: &SigningRequest) -> Result<UnsignedTransaction, Failu
scheme: Scheme::Ed25519,
}]
}
(
TransactionSpec::Tron {
raw_data_hex,
expected_to,
expected_txid,
},
Chain::Tron,
) => {
TransactionSpec::Tron {
raw_data_hex,
expected_to,
expected_txid,
} => {
// Tron's node builds the transaction, so the only defence against a
// compromised endpoint is checking that what came back is what was
// asked for — before signing it, which is here.
Expand All @@ -161,25 +153,24 @@ fn build_unsigned(request: &SigningRequest) -> Result<UnsignedTransaction, Failu
tx::tron::digest(raw_data_hex).map_err(|e| build_failed(&e))?,
)]
}
(spec, chain) => return Err(mismatched(spec, chain)),
// Required because `TransactionSpec` is `#[non_exhaustive]`: a shape
// added after this build must be refused, never guessed at.
_ => return Err(unknown_kind()),
};
Ok(UnsignedTransaction { payloads })
}

/// Assemble the signed transaction for `request`.
fn attach_signature(request: &AttachRequest) -> Result<SignedTransaction, Failure> {
match (&request.transaction, request.chain) {
(
TransactionSpec::Btc {
from,
to,
amount_sat,
fee_rate_sat_vb,
utxos,
},
Chain::Btc,
) => {
let transfer = btc_transfer(from, to, *amount_sat, *fee_rate_sat_vb);
match &request.transaction {
TransactionSpec::Btc {
from,
to,
amount_sat,
fee_sat,
utxos,
} => {
let transfer = btc_transfer(from, to, *amount_sat, *fee_sat);
let public = compressed_public_key(&request.public_key.key_hex)?;
let signatures = request
.signatures
Expand All @@ -198,7 +189,7 @@ fn attach_signature(request: &AttachRequest) -> Result<SignedTransaction, Failur
raw,
})
}
(spec @ TransactionSpec::Evm { .. }, Chain::Evm) => {
spec @ TransactionSpec::Evm { .. } => {
let (rs, recovery) = single_secp256k1(&request.signatures)?;
let signed = evm_transaction(spec)?
.attach_signature(&rs, recovery)
Expand All @@ -208,7 +199,7 @@ fn attach_signature(request: &AttachRequest) -> Result<SignedTransaction, Failur
raw: format!("0x{}", hex(&signed)),
})
}
(spec @ TransactionSpec::Solana { .. }, Chain::Solana) => {
spec @ TransactionSpec::Solana { .. } => {
let signature = single_ed25519(&request.signatures)?;
let signed = solana_transfer(spec)?
.attach_signature(&signature)
Expand All @@ -221,14 +212,11 @@ fn attach_signature(request: &AttachRequest) -> Result<SignedTransaction, Failur
raw: base64(&signed),
})
}
(
TransactionSpec::Tron {
raw_data_hex,
expected_to,
expected_txid,
},
Chain::Tron,
) => {
TransactionSpec::Tron {
raw_data_hex,
expected_to,
expected_txid,
} => {
// Verified again rather than trusted from the first call: the two
// requests are independent, and a host could reach this one with
// different bytes than the digest was computed over.
Expand All @@ -242,34 +230,17 @@ fn attach_signature(request: &AttachRequest) -> Result<SignedTransaction, Failur
raw: tx::tron::signature_hex(&signature),
})
}
(spec, chain) => Err(mismatched(spec, chain)),
_ => Err(unknown_kind()),
}
}

/// A `chain` tag that does not agree with the transaction it carries.
fn mismatched(spec: &TransactionSpec, chain: Chain) -> Failure {
let named = match spec {
TransactionSpec::Btc { .. } => Chain::Btc,
TransactionSpec::Evm { .. } => Chain::Evm,
TransactionSpec::Solana { .. } => Chain::Solana,
TransactionSpec::Tron { .. } => Chain::Tron,
// `TransactionSpec` is `#[non_exhaustive]`, so a variant added later
// must land here rather than failing to compile in a crate that cannot
// see it. Refusing is the safe direction: never sign an unknown shape.
_ => {
return Failure::InvalidInput(
"this build does not understand that transaction kind".to_string(),
);
}
};
if named == chain {
// Same chain on both sides, so the pairing failed for the only other
// reason: this build does not carry it.
return Failure::UnsupportedChain(chain);
}
Failure::InvalidInput(format!(
"the request names {chain} but carries a {named} transaction"
))
/// The refusal for a transaction shape added after this build.
///
/// `TransactionSpec` is `#[non_exhaustive]`, so a peer built against a newer
/// revision can send a variant this module cannot name. Refusing beats
/// guessing: the alternative is building some other chain's transaction.
fn unknown_kind() -> Failure {
Failure::InvalidInput("this build does not understand that transaction kind".to_string())
}

/// Collapse a `tinywallet` build error, which is never the caller's fault by
Expand Down
63 changes: 42 additions & 21 deletions crates/tinywallet-module/src/service/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ fn the_split_path_reproduces_a_one_shot_signature() {
let spec = evm_spec();

let unsigned = build_unsigned(&SigningRequest {
chain: Chain::Evm,
transaction: spec.clone(),
public_key: PublicKey {
key_hex: compressed_public(&key),
Expand All @@ -87,7 +86,6 @@ fn the_split_path_reproduces_a_one_shot_signature() {

let signature = host_sign(&unsigned.payloads[0].bytes_hex, &key);
let signed = attach_signature(&AttachRequest {
chain: Chain::Evm,
transaction: spec,
public_key: PublicKey {
key_hex: compressed_public(&key),
Expand Down Expand Up @@ -124,7 +122,7 @@ fn a_bitcoin_request_returns_one_payload_per_selected_input() {
from: key.address().to_string(),
to: "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4".to_string(),
amount_sat: 150_000,
fee_rate_sat_vb: 2_000,
fee_sat: 2_000,
utxos: vec![
Utxo {
txid: "7f3b662ea8b6ff2e0e1a1f9bd0f1c39a6b8ba51e1b0f0e0d0c0b0a0908070605"
Expand All @@ -148,7 +146,6 @@ fn a_bitcoin_request_returns_one_payload_per_selected_input() {
};

let unsigned = build_unsigned(&SigningRequest {
chain: Chain::Btc,
transaction: spec,
public_key: PublicKey {
key_hex: compressed_public(key.secret_bytes()),
Expand All @@ -172,7 +169,6 @@ fn a_solana_payload_is_the_message_not_a_digest() {
// the network rejects, so the scheme tag has to say so.
let key = tinywallet::key::derive(Chain::Solana, VECTOR, "m/44'/501'/0'/0'").unwrap();
let unsigned = build_unsigned(&SigningRequest {
chain: Chain::Solana,
transaction: TransactionSpec::Solana {
from: key.address().to_string(),
to: "11111111111111111111111111111111".to_string(),
Expand All @@ -193,26 +189,53 @@ fn a_solana_payload_is_the_message_not_a_digest() {
}

#[test]
fn a_chain_tag_that_disagrees_with_its_transaction_is_refused() {
// The tag and the fields are independent on the wire, so the mismatch is
// reachable and must not be resolved by guessing which one is right.
let error = build_unsigned(&SigningRequest {
chain: Chain::Btc,
transaction: evm_spec(),
public_key: PublicKey {
key_hex: compressed_public(&evm_key()),
},
})
.unwrap_err();
fn the_chain_comes_from_the_transaction_rather_than_a_separate_field() {
// This replaces a test that fed a request whose `chain` tag contradicted
// its transaction. That state is no longer expressible: the requests carry
// no `chain`, so `TransactionSpec` is the single source of truth and the
// disagreement cannot be constructed. What is worth pinning instead is
// that the mapping is right for every variant.
use tinywallet::wire::Utxo;

let rendered = format!("{error:?}");
assert!(rendered.contains("InvalidInput"), "{rendered}");
let cases = [
(
TransactionSpec::Btc {
from: String::new(),
to: String::new(),
amount_sat: 0,
fee_sat: 0,
utxos: Vec::<Utxo>::new(),
},
Chain::Btc,
),
(evm_spec(), Chain::Evm),
(
TransactionSpec::Solana {
from: String::new(),
to: String::new(),
lamports: 0,
recent_blockhash: String::new(),
},
Chain::Solana,
),
(
TransactionSpec::Tron {
raw_data_hex: String::new(),
expected_to: String::new(),
expected_txid: String::new(),
},
Chain::Tron,
),
];

for (spec, expected) in cases {
assert_eq!(spec.chain(), expected);
}
}

#[test]
fn an_ed25519_signature_is_refused_for_a_secp256k1_chain() {
let error = attach_signature(&AttachRequest {
chain: Chain::Evm,
transaction: evm_spec(),
public_key: PublicKey {
key_hex: compressed_public(&evm_key()),
Expand All @@ -230,7 +253,6 @@ fn an_ed25519_signature_is_refused_for_a_secp256k1_chain() {
#[test]
fn a_wrong_signature_count_is_refused_rather_than_truncated() {
let error = attach_signature(&AttachRequest {
chain: Chain::Evm,
transaction: evm_spec(),
public_key: PublicKey {
key_hex: compressed_public(&evm_key()),
Expand All @@ -248,7 +270,6 @@ fn a_tron_transaction_whose_txid_does_not_match_its_bytes_is_refused() {
// The defence against a compromised node: it must not be possible to get a
// signature over bytes whose recomputed id disagrees with what was claimed.
let error = build_unsigned(&SigningRequest {
chain: Chain::Tron,
transaction: TransactionSpec::Tron {
raw_data_hex: "0a02b1f12208".to_string() + &"ab".repeat(64),
expected_to: "TR7NHqjeKQxGTCi8q8ZY4pL8otSzgjLj6t".to_string(),
Expand Down
Loading