test(txbuilder): add a Conway build test harness#781
Open
logicalmechanism wants to merge 8 commits into
Open
Conversation
…coverage Introduce an integration test suite for the Conway builder. This first commit covers the simplest transaction (one input, one output, a fee) and establishes the oracles the rest of the suite will reuse: - decode round-trip via pallas_primitives::conway::Tx - a golden CBOR + hash snapshot under tests/golden/ - a determinism check (same inputs => same bytes) No library changes; pure test additions plus the dev-dependencies the integration test crate needs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Group B of the Conway build suite: - sign() embeds exactly one vkey witness and records it in `signatures` - signing preserves the body hash (witnesses don't change what was signed) - signing twice accumulates two witnesses - add_signature embeds an out-of-band witness (HSM / hardware-wallet flow) - remove_signature leaves the remaining witnesses intact Signatures are produced from a deterministic, clamped extended Ed25519 key so the suite needs no RNG. This surfaced a panic: remove_signature on the *only* witness unwraps `NonEmptySet::from_vec(vec![])` (None) instead of clearing the set. It is captured as an #[ignore]d test (remove_last_signature_clears_witness_set) to be un-ignored by the PR that fixes the panic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Group C of the Conway build suite: - multi-asset outputs build deterministically (asset bundles encode through BTreeMap-backed Multiasset, so order is normalised) - mint bundles build deterministically for the same reason And document the determinism gap that is not normalised: datums (like redeemers and scripts) are stored in a HashMap and emitted in iteration order, so the same transaction can encode to different bytes/hash across builds. Captured as an #[ignore]d test (multiple_datums_build_deterministically) that builds the tx fresh 64 times and asserts a single unique encoding; to be un-ignored by the PR that sorts these collections before encoding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add Group D of the Conway build suite: a transaction with a native token in its output plus a mint and a burn. Decode the result and assert the lovelace, asset name/quantity, mint amount, and burn amount survive the round-trip, and pin a golden CBOR + hash snapshot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion Add Group E of the Conway build suite, mirroring the self-consistency check pallas-primitives runs on real transactions: build a tx with datums, a spend redeemer, and a cost model, then assert the embedded script_data_hash equals the value recomputed from the decoded witness set via ScriptData::build_for. Also document the gating bug: the builder computes script_data_hash only when language_views is set, but the ledger requires it whenever the witness set carries datums or redeemers. A datum-only transaction silently gets None and is rejected by the node. Captured as an #[ignore]d test (datum_only_sets_script_data_hash); to be un-ignored by the PR that switches the builder to ScriptData::build_for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ions Add Group F: reconstruct three real, unsigned Conway transactions produced by cardano-cli (stored under tests/vectors/) through the builder and assert the results match. - datum_output: inline-datum payment + change - token_output: two inputs, inline-datum + token output, plain and token change - native_mint: native-script mint, token output + change, script in the witness set cardano-cli emits plain/token outputs in the legacy [address, value] array form while the builder always emits the post-Alonzo map form (both valid per the Conway CDDL), so the comparison is semantic — inputs, fee, validity bounds, mint, native scripts, and per-output address/coin/assets/datum — and additionally requires outputs that are post-Alonzo in both to be byte-identical. The inline-datum output reproduces byte-for-byte, including the indefinite-array datum encoding. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a fourth real cardano-cli fixture (tests/vectors/plutus_spend_burn.tx): a Plutus spend whose script is supplied via a reference input, with a spend redeemer, a mint (burn) redeemer, collateral, collateral return, and required signers. Extend the comparator to also check collateral inputs, reference inputs, required signers, and the collateral-return output — all of which the builder reproduces faithfully here. Two things the builder cannot yet reproduce are captured as #[ignore]d tests documenting the gaps: - plutus_tx_sets_total_collateral: the builder hardcodes total_collateral to None with no setter, so it drops the field (fixture sets 579653). - plutus_tx_emits_map_form_redeemers: the builder emits redeemers in the legacy list form, while Conway cardano-cli uses the map form; the two hash differently, so the builder's script_data_hash is incompatible with cli. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review feedback on the Conway build test harness: - Strengthen multi_asset_output_is_deterministic / mint_is_deterministic to assert a single unique encoding across 64 fresh builds (collected into a HashSet) and to stage two asset names under one policy, so both the outer (policy) and inner (asset-name) BTreeMap sorts are exercised. Each build constructs a fresh, per-instance re-seeded HashMap, which is what drives the pre-encode order variation; the previous form compared one fixed-order build to itself and never varied the bundle order. - Assert bundle sizes in output_asset_value_round_trips and mint_and_burn_round_trip so a spuriously added asset/policy cannot slip past the single-entry .next() checks. - Reword the script_data_hash_matches_recomputation doc comment to make clear it is a self-consistency check, not a cross-check against cardano-cli (it reuses the builder's legacy list-form redeemer encoding). - Remove the [dev-dependencies] block from pallas-txbuilder/Cargo.toml: pallas-crypto, pallas-primitives, pallas-addresses, and hex are all already in [dependencies], which Cargo makes available to integration tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR adds a comprehensive integration test suite ( ChangesConway Transaction Builder Integration Test Suite
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First in a planned series of small PRs hardening
pallas-txbuilder. This one adds an integration-test harness for the Conway builder; later PRs fix the bugs it documents (see Known-bug documentation below).What
Adds
tests/build_conway.rsplus golden snapshots and cardano-cli vectors, exercisingbuild_conway_rawandBuiltTransactionsigning with no live node required. No production code changes — the harness rides on the crate's existing[dependencies].Test oracles:
pallas_primitives::conway::Txand asserted field-by-field against the builder inputs.tests/golden/, so any encoder change shows up as a diff.BTreeMapnormalisation of asset/mint bundles at both the policy and asset-name level (the staging maps areHashMap-backed and re-seeded per build).script_data_hashis recomputed from the decoded witness set viaScriptData::build_forand asserted equal.tests/vectors/are reconstructed by the builder and compared field-by-field: inputs, outputs, fee, validity bounds, mint, native scripts, collateral, reference inputs, and required signers.Known-bug documentation
Five tests are
#[ignore]d on purpose: they fail when un-ignored and pin down current builder bugs as executable specs, each with a comment citing the exact source location. Defaultcargo teststays green, so CI is unaffected. Each is intended to be un-ignored by the follow-up PR that fixes it:script_data_hashomitted (gated onlanguage_views)total_collateralhardcoded toNonewith no setterremove_signaturepanics when emptying the witness set🤖 Generated with Claude Code
Summary by CodeRabbit