fix: get the tx_id from the transaction body#823
Conversation
WalkthroughThis PR centralizes transaction identifiers in the kernel by introducing ChangesKernel: TransactionId + HasTransactionId
Mempool traits & values migration
Mempool implementations & strategies
Protocols, tx-submission, and effects
Consensus, mempool stage, and other consumers
Sequence Diagram sequenceDiagram
autonumber
participant Client as Client
participant Tx as Transaction
participant Kernel as Kernel (TransactionId)
participant Mempool as Mempool
participant Protocol as TxSubmission
participant Ledger as Ledger
Client->>Tx: create tx
Tx->>Kernel: tx.tx_id() (compute TransactionId)
Note right of Kernel: TransactionId is opaque wrapper\naround TransactionIdHash (first 6 bytes shown in display)
Tx->>Mempool: submit tx (+TransactionId via tx.tx_id())
Mempool->>Protocol: advertise/request using TransactionId
Protocol->>Ledger: validation may return Invalid(transaction_hash)
Ledger-->>Mempool: validation result (mapped to TxInsertResult::rejected with tx.tx_id())
Mempool-->>Client: insertion result (Accepted/Rejected with TransactionId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
crates/amaru-kernel/src/cardano/transaction.rs (1)
50-51: ⚡ Quick winAdd
#[repr(transparent)]onTxIdnewtype.Small but useful ABI/layout guarantee for this domain newtype, mate.
Proposed tweak
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize)] +#[repr(transparent)] pub struct TxId(TransactionId);As per coding guidelines, “Implement newtypes with:
#[derive(...)] #[repr(transparent)] pub struct Name(Type);with private field...”.🤖 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/amaru-kernel/src/cardano/transaction.rs` around lines 50 - 51, Add the #[repr(transparent)] attribute to the TxId newtype declaration to provide a stable ABI/layout guarantee for this domain newtype; update the declaration that currently reads "pub struct TxId(TransactionId);" (the TxId newtype around TransactionId) by prepending #[repr(transparent)] while keeping the existing derives and the inner field private.
🤖 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.
Nitpick comments:
In `@crates/amaru-kernel/src/cardano/transaction.rs`:
- Around line 50-51: Add the #[repr(transparent)] attribute to the TxId newtype
declaration to provide a stable ABI/layout guarantee for this domain newtype;
update the declaration that currently reads "pub struct TxId(TransactionId);"
(the TxId newtype around TransactionId) by prepending #[repr(transparent)] while
keeping the existing derives and the inner field private.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de148785-9279-41dc-9ebe-4174738bffc1
📒 Files selected for processing (19)
crates/amaru-consensus/src/stages/mempool/stage.rscrates/amaru-consensus/src/stages/mempool/tests.rscrates/amaru-kernel/src/cardano/transaction.rscrates/amaru-kernel/src/lib.rscrates/amaru-mempool/src/strategies/dummy.rscrates/amaru-mempool/src/strategies/in_memory_mempool.rscrates/amaru-mempool/src/strategies/mod.rscrates/amaru-ouroboros-traits/src/mempool/mempool_traits.rscrates/amaru-ouroboros-traits/src/mempool/values.rscrates/amaru-ouroboros/src/mempool.rscrates/amaru-protocols/src/mempool_effects.rscrates/amaru-protocols/src/tests/configuration.rscrates/amaru-protocols/src/tx_submission/initiator.rscrates/amaru-protocols/src/tx_submission/messages.rscrates/amaru-protocols/src/tx_submission/outcome.rscrates/amaru-protocols/src/tx_submission/responder.rscrates/amaru-protocols/src/tx_submission/tests/assertions.rscrates/amaru/src/submit_api.rscrates/amaru/src/tests/configuration.rs
💤 Files with no reviewable changes (2)
- crates/amaru-mempool/src/strategies/mod.rs
- crates/amaru-mempool/src/strategies/dummy.rs
etorreborre
force-pushed
the
etorreborre/fix/transaction-id
branch
from
dd5476a to
a7226ed
Compare
| /// It encapsulates the TransactionId to be completely opaque to the structure of the | ||
| /// transaction identifier. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize)] | ||
| pub struct TxId(TransactionId); |
There was a problem hiding this comment.
Please make a new kernel module 🙏, one module per type.
Also, let's try to avoid acronyms and short names as much as we can; 👉 TransactionId
There was a problem hiding this comment.
There's already a TransactionId used as a type alias for the hash. Should I remove that type alias then?
There was a problem hiding this comment.
I pushed a new commit doing that renaming and then renaming the original TransactionId to TransactionIdHash. However it's not consistent with PoolId which is also a hash. Are you ok to rename it to PoolIdHash or do you have another naming scheme that you'd prefer?
There was a problem hiding this comment.
TransactionIdHash
😬 ... transaction id is already a hash. So; yeah, we should NOT use a type-alias if we think this deserves to be a proper type with its own set of methods.
It doesn't make sense to me to have TransactionId and TransactionIdHash (same for pool).
There was a problem hiding this comment.
Looking at the code here though; I don't really see what's specific to TransactionId though; everything could be generally handled by the Hash (though we don't yet control this type, so can't really extend it).
|
|
||
| impl Display for TxId { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
| write!(f, "{}", hex::encode(self.0.as_slice())) |
There was a problem hiding this comment.
Might want to limit to the first 10-12 chars.
rkuhn
left a comment
rkuhn
left a comment
There was a problem hiding this comment.
It looks like shuffling around rather similarly named types in some places, but I fail to see an actual change of computational rules. I’m a bit confused by the intended difference between TxId and TransactionId, which sounds like they should be the same thing to begin with.
etorreborre
force-pushed
the
etorreborre/fix/transaction-id
branch
2 times, most recently
from
8759ca9 to
3ed400f
Compare
Signed-off-by: Eric Torreborre <[email protected]>
and TransactionId to TransactionIdHash Signed-off-by: Eric Torreborre <[email protected]>
etorreborre
force-pushed
the
etorreborre/fix/transaction-id
branch
from
3ed400f to
31fa2b7
Compare
Signed-off-by: Eric Torreborre <[email protected]>
etorreborre
force-pushed
the
etorreborre/fix/transaction-id
branch
from
31fa2b7 to
e5b1e11
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/amaru-kernel/src/cardano/transaction_id.rs`:
- Around line 27-28: The TransactionId newtype is missing the
#[repr(transparent)] attribute; add #[repr(transparent)] immediately above the
TransactionId declaration so the struct (pub struct
TransactionId(TransactionIdHash);) is represented transparently over its single
private field, keeping the existing derives intact (Debug, Copy, Clone,
PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize) to
comply with the repo newtype guideline.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b50769b8-be96-416b-89dd-f9237c1ae535
📒 Files selected for processing (28)
crates/amaru-consensus/src/stages/mempool/stage.rscrates/amaru-consensus/src/stages/mempool/tests.rscrates/amaru-kernel/src/cardano.rscrates/amaru-kernel/src/cardano/hash.rscrates/amaru-kernel/src/cardano/transaction.rscrates/amaru-kernel/src/cardano/transaction_id.rscrates/amaru-kernel/src/lib.rscrates/amaru-kernel/src/traits.rscrates/amaru-kernel/src/traits/has_transaction_id.rscrates/amaru-ledger/src/rules/block.rscrates/amaru-ledger/src/rules/transaction/phase_one/proposals.rscrates/amaru-ledger/src/rules/transaction/phase_one/vkey_witness.rscrates/amaru-mempool/src/strategies/dummy.rscrates/amaru-mempool/src/strategies/in_memory_mempool.rscrates/amaru-mempool/src/strategies/mod.rscrates/amaru-ouroboros-traits/src/mempool/mempool_traits.rscrates/amaru-ouroboros-traits/src/mempool/values.rscrates/amaru-ouroboros/src/mempool.rscrates/amaru-plutus/src/script_context/mod.rscrates/amaru-protocols/src/mempool_effects.rscrates/amaru-protocols/src/tests/configuration.rscrates/amaru-protocols/src/tx_submission/initiator.rscrates/amaru-protocols/src/tx_submission/messages.rscrates/amaru-protocols/src/tx_submission/outcome.rscrates/amaru-protocols/src/tx_submission/responder.rscrates/amaru-protocols/src/tx_submission/tests/assertions.rscrates/amaru/src/submit_api.rscrates/amaru/src/tests/configuration.rs
💤 Files with no reviewable changes (2)
- crates/amaru-mempool/src/strategies/mod.rs
- crates/amaru-mempool/src/strategies/dummy.rs
✅ Files skipped from review due to trivial changes (2)
- crates/amaru-kernel/src/traits/has_transaction_id.rs
- crates/amaru-protocols/src/tx_submission/tests/assertions.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/amaru-protocols/src/tests/configuration.rs
- crates/amaru/src/submit_api.rs
- crates/amaru-consensus/src/stages/mempool/tests.rs
- crates/amaru-consensus/src/stages/mempool/stage.rs
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize)] | ||
| pub struct TransactionId(TransactionIdHash); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add #[repr(transparent)] on TransactionId for newtype compliance.
Quick win, mate: Line 28 defines a domain newtype, but it misses #[repr(transparent)], which this repo asks for on newtypes.
Proposed patch
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize)]
+#[repr(transparent)]
pub struct TransactionId(TransactionIdHash);As per coding guidelines: "Implement newtypes with: #[derive(...)] #[repr(transparent)] pub struct Name(Type); with private field, and derive Debug, Clone, Copy (if applicable), PartialEq/Eq/Ord, Display, Serialize/Deserialize, CBOR Encode/Decode".
📝 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.
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize)] | |
| pub struct TransactionId(TransactionIdHash); | |
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize, serde::Deserialize)] | |
| #[repr(transparent)] | |
| pub struct TransactionId(TransactionIdHash); |
🤖 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/amaru-kernel/src/cardano/transaction_id.rs` around lines 27 - 28, The
TransactionId newtype is missing the #[repr(transparent)] attribute; add
#[repr(transparent)] immediately above the TransactionId declaration so the
struct (pub struct TransactionId(TransactionIdHash);) is represented
transparently over its single private field, keeping the existing derives intact
(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord, serde::Serialize,
serde::Deserialize) to comply with the repo newtype guideline.
3 participants
The main fix of this PR is the retrieval of the transaction id used in the mempool and tx submission protocol from the transaction body and not from the hashing of the full transaction CBOR.
Then, in order to make the access to the transaction id more consistent and encapsulated, I defined a
tx_id()method onTransaction.Notes:
dummymempool in this PR instead of having to maintain it.Summary by CodeRabbit
TransactionIdtype across consensus, mempool, and protocol layers.