feat(trp): update tx3 to latest version #768
Conversation
WalkthroughChanges introduce a Facade wrapper struct providing domain data access methods (chain summary, slot config, chain tip, pparams), refactor load_compiler to accept the wrapped domain and return tx3_cardano::Compiler, add two new error variants, extend UtxoStore with fetch_utxos_deps, and update Cargo.toml dependencies to git-based sources for tx3-* crates. Changes
Sequence DiagramsequenceDiagram
participant C as Consumer
participant M as methods
participant F as Facade
participant CMP as compiler
participant D as Domain
Note over C,D: Previous Flow
C->>M: trp_resolve()
M->>CMP: load_compiler(domain)
CMP->>D: build_pparams()
Note over C,D: New Flow
C->>M: trp_resolve()
M->>F: Facade::new(domain)
M->>CMP: load_compiler(&facade)
CMP->>F: get_chain_summary()
F->>D: retrieve era summary
CMP->>F: get_slot_config()
CMP->>F: get_chain_tip()
F->>D: read state cursor
CMP->>F: get_pparams()
F->>D: load effective pparams
CMP-->>C: Return tx3_cardano::Compiler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
crates/trp/Cargo.toml (1)
27-37: Pin tx3 git dependencies for reproducible builds; consider aligning tx3-sdk source.Moving to git HEADs risks non-deterministic builds and surprise API breaks. Pin commits and keep all tx3 crates in lockstep. Also consider pinning tx3-sdk or switching it to the same repo/rev if required by the new APIs.
Apply something like:
-tx3-lang = { git = "https://github.com/tx3-lang/tx3.git" } -tx3-cardano = { git = "https://github.com/tx3-lang/tx3.git" } -tx3-resolver = { git = "https://github.com/tx3-lang/tx3.git" } +tx3-lang = { git = "https://github.com/tx3-lang/tx3.git", rev = "<commit-sha>" } +tx3-cardano = { git = "https://github.com/tx3-lang/tx3.git", rev = "<same-commit-sha>" } +tx3-resolver = { git = "https://github.com/tx3-lang/tx3.git", rev = "<same-commit-sha>" }Optionally, align SDK:
-tx3-sdk = { version = "^0" } +# tx3-sdk = { git = "https://github.com/tx3-lang/rust-sdk.git", rev = "<sdk-commit-sha>" }crates/trp/src/error.rs (1)
56-61: New variants look good; consider adding error data for client diagnostics.TipNotResolved and ChainSummaryNotResolved are appropriate. Add a small JSON payload in data() so clients can programmatically inspect causes (e.g., the underlying summary error string).
Example addition in data():
_ => None, + Error::TipNotResolved => None, + Error::ChainSummaryNotResolved(msg) => { + Some(json!({ "reason": msg })) + }crates/trp/src/methods.rs (1)
11-11: Facade integration is correct; minor cohesion nit.Constructing Facade locally and passing it to load_compiler is fine. For consistency, consider also deriving the state via the same Facade (or clone the same domain into both) to avoid mixing domain clones, though it’s likely harmless if D internally Arc’s state.
Example:
- let facade = Facade { inner: context.domain.clone() }; - let mut compiler = load_compiler::<D>(&facade, &context.config)?; - let utxos = UtxoStoreAdapter::<D>::new(context.domain.state().clone()); + let facade = Facade { inner: context.domain.clone() }; + let mut compiler = load_compiler::<D>(&facade, &context.config)?; + let utxos = UtxoStoreAdapter::<D>::new(facade.state().clone());Also applies to: 80-83
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/trp/Cargo.toml(1 hunks)crates/trp/src/compiler.rs(2 hunks)crates/trp/src/error.rs(3 hunks)crates/trp/src/lib.rs(2 hunks)crates/trp/src/methods.rs(2 hunks)crates/trp/src/utxos.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/trp/src/methods.rs (2)
crates/trp/src/compiler.rs (1)
load_compiler(32-58)crates/minibf/src/routes/addresses.rs (1)
utxos(128-148)
crates/trp/src/lib.rs (3)
src/mempool.rs (4)
dolos_cardano(85-85)dolos_cardano(133-133)dolos_cardano(135-135)validate(67-124)crates/cardano/src/lib.rs (3)
eras(153-153)eras(178-178)load_effective_pparams(327-332)crates/core/src/lib.rs (3)
chain(712-712)slot(497-497)hash(498-498)
crates/trp/src/utxos.rs (2)
src/mempool.rs (1)
validate(67-124)crates/trp/src/mapping.rs (1)
from_tx3_utxoref(99-103)
🪛 GitHub Actions: CI
crates/trp/src/utxos.rs
[error] 80-80: method fetch_utxos_deps is not a member of trait UtxoStore.
🪛 GitHub Check: Check Build
crates/trp/src/compiler.rs
[failure] 46-46:
this function takes 3 arguments but 6 arguments were supplied
crates/trp/src/utxos.rs
[failure] 80-80:
method fetch_utxos_deps is not a member of trait UtxoStore
🔇 Additional comments (5)
crates/trp/src/utxos.rs (1)
8-9: LGTM!The new imports support the
fetch_utxos_depsfunctionality and are appropriately scoped.crates/trp/src/error.rs (1)
137-139: JSON‑RPC code mapping consistent.Codes are in the custom range and mapped correctly in code(). LGTM.
Also applies to: 160-162
crates/trp/src/lib.rs (2)
3-7: Imports look correct and scoped.New uses for ChainPoint, ChainSummary, PParamsSet, and SlotConfig are appropriate.
Also applies to: 13-17
149-156: Timestamp and hash handling contain real bugs; fix approach is sound but requires verification of external type compatibility.The review comment correctly identifies two issues:
Timestamp calculation bug: The current
(slot * 1000)is a placeholder (indicated by TODO). The codebase includesEraSummary.slot_time()which correctly computesself.start.timestamp + (slot - self.start.slot) * self.slot_length, validating the reviewer's suggested approach.Hash error handling: Using
unwrap_or_default()silently converts missing hash to an empty vector. RaisingError::TipNotResolvedis more explicit and correct.However, the proposed diff cannot be fully verified because tx3_cardano re-exports
ChainPointfrom the Pallas crate, and the exact field names and types inpallas::interop::utxorpc::spec::query::ChainPointcould not be confirmed. The diff assumes fieldsslot,hash, andtimestampwith matching types, but this must be verified against the Pallas crate definition before applying the fix.crates/trp/src/compiler.rs (1)
46-55: Compiler::new arity mismatch identified, but review comment contains incorrect solution patterns.The actual tx3-cardano 0.11.4
Compiler::newsignature is:pub fn new(pparams: PParams, config: Config) -> SelfIt takes 2 arguments, not 3. The current call at lines 46-55 passes 6 arguments, causing the build to fail. The correct call should be:
let compiler = tx3_cardano::Compiler::new( pparams, tx3_cardano::Config { extra_fees: config.extra_fees, }, );The parameters
network,chain_tip,map_cost_models(costs), andslot_configshould be removed from this call. Verify they are either no longer needed or set elsewhere in the flow.Likely an incorrect or invalid review comment.
| domain: &Facade<D>, | ||
| config: &Config, | ||
| ) -> Result<tx3_cardano::Compiler, Error> { | ||
| let network = network_id_from_genesis(&domain.genesis()).unwrap(); |
There was a problem hiding this comment.
Don’t unwrap network id; propagate a typed error.
Unwrap will panic on unknown/missing network_id. Map to an Error instead.
- let network = network_id_from_genesis(&domain.genesis()).unwrap();
+ let network = network_id_from_genesis(&domain.genesis())
+ .ok_or_else(|| {
+ jsonrpsee::types::ErrorObjectOwned::owned(
+ jsonrpsee::types::ErrorCode::InternalError.code(),
+ "unsupported or missing network_id in genesis",
+ None::<serde_json::Value>,
+ )
+ })?
+ .into();Note: Using JsonRpcError keeps behavior consistent with existing Error conversions.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/trp/src/compiler.rs around line 36, replace the unwrap on
network_id_from_genesis(&domain.genesis()) with error propagation: call
network_id_from_genesis(...)? or map its None/Err into the crate Error using
JsonRpcError (or convert into the existing Error enum) and return it instead of
panicking; ensure the function signature returns Result so the propagated error
flows up and maintain existing Error conversion behavior by constructing a
JsonRpcError with a clear message when network id is missing/unknown.
| pub fn get_chain_summary(&self) -> Result<ChainSummary, error::Error> { | ||
| let summary = dolos_cardano::eras::load_era_summary::<D>(&self.inner.state()) | ||
| .map_err(|e| error::Error::ChainSummaryNotResolved(e.to_string()))?; | ||
|
|
||
| Ok(summary) | ||
| } |
There was a problem hiding this comment.
Remove extra reference when passing state().
state() likely already returns a reference; passing &state() produces &&State. Call with state() directly.
- let summary = dolos_cardano::eras::load_era_summary::<D>(&self.inner.state())
+ let summary = dolos_cardano::eras::load_era_summary::<D>(self.inner.state())
.map_err(|e| error::Error::ChainSummaryNotResolved(e.to_string()))?;📝 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.
| pub fn get_chain_summary(&self) -> Result<ChainSummary, error::Error> { | |
| let summary = dolos_cardano::eras::load_era_summary::<D>(&self.inner.state()) | |
| .map_err(|e| error::Error::ChainSummaryNotResolved(e.to_string()))?; | |
| Ok(summary) | |
| } | |
| pub fn get_chain_summary(&self) -> Result<ChainSummary, error::Error> { | |
| let summary = dolos_cardano::eras::load_era_summary::<D>(self.inner.state()) | |
| .map_err(|e| error::Error::ChainSummaryNotResolved(e.to_string()))?; | |
| Ok(summary) | |
| } |
🤖 Prompt for AI Agents
crates/trp/src/lib.rs around lines 132 to 137: the call currently passes
&self.inner.state() which creates a double reference (&&State); remove the extra
reference and pass self.inner.state() directly to
dolos_cardano::eras::load_era_summary::<D>() so the function receives the
expected &State (or move) value. Update the call to use self.inner.state()
without the leading & and keep the map_err handling intact.
| pub fn get_pparams(&self) -> Result<PParamsSet, error::Error> { | ||
| let pparams = dolos_cardano::load_effective_pparams::<D>(&self.inner.state()) | ||
| .map_err(|_| Error::PParamsNotAvailable)?; | ||
|
|
||
| Ok(pparams) | ||
| } |
There was a problem hiding this comment.
Same extra reference issue for pparams.
Pass state() directly; also consider preserving the source error message.
- let pparams = dolos_cardano::load_effective_pparams::<D>(&self.inner.state())
- .map_err(|_| Error::PParamsNotAvailable)?;
+ let pparams = dolos_cardano::load_effective_pparams::<D>(self.inner.state())
+ .map_err(|e| {
+ tracing::debug!(error = %e, "effective pparams not available");
+ Error::PParamsNotAvailable
+ })?;📝 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.
| pub fn get_pparams(&self) -> Result<PParamsSet, error::Error> { | |
| let pparams = dolos_cardano::load_effective_pparams::<D>(&self.inner.state()) | |
| .map_err(|_| Error::PParamsNotAvailable)?; | |
| Ok(pparams) | |
| } | |
| pub fn get_pparams(&self) -> Result<PParamsSet, error::Error> { | |
| let pparams = dolos_cardano::load_effective_pparams::<D>(self.inner.state()) | |
| .map_err(|e| { | |
| tracing::debug!(error = %e, "effective pparams not available"); | |
| Error::PParamsNotAvailable | |
| })?; | |
| Ok(pparams) | |
| } |
🤖 Prompt for AI Agents
In crates/trp/src/lib.rs around lines 158 to 163, the code passes
&self.inner.state() producing an extra reference and also discards the original
error; call dolos_cardano::load_effective_pparams::<D>(self.inner.state()) (i.e.
pass state() directly, removing the &), and when mapping the error convert it
while preserving context by using map_err(|e|
Error::PParamsNotAvailable.into_with_source(e)) or otherwise include the source
error in the Error::PParamsNotAvailable construction so the original error
message is retained in the returned Result.
| async fn fetch_utxos_deps( | ||
| &self, | ||
| refs: HashSet<UtxoRef> | ||
| ) -> Result<UtxoMap, tx3_lang::backend::Error> { | ||
| let refs: Vec<_> = refs.into_iter().map(from_tx3_utxoref).collect(); | ||
|
|
||
| let utxos = self.state().get_utxos(refs).map_err(Error::from)?; | ||
|
|
||
| let utxos = utxos | ||
| .into_iter() | ||
| .map(|(txoref, eracbor)| { | ||
| let TxoRef(a, b) = txoref; | ||
| let EraCbor(c, d) = eracbor.as_ref(); | ||
| let era = pallas::ledger::traverse::Era::try_from(*c).expect("era out of range"); | ||
|
|
||
| ( | ||
| pallas::ledger::validate::utils::TxoRef::from((a, b)), | ||
| pallas::ledger::validate::utils::EraCbor::from((era, d.clone())), | ||
| ) | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(utxos) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Method not part of trait UtxoStore.
The implementation cannot compile because fetch_utxos_deps is not defined in the tx3_lang::backend::UtxoStore trait. This indicates either:
- The trait definition hasn't been updated to include this method
- The tx3-lang dependency version is incorrect or not yet updated
Verify that the tx3-lang dependency has been updated to a version that includes this method in the trait definition.
Run the following script to check the trait definition:
🏁 Script executed:
#!/bin/bash
# Description: Verify the UtxoStore trait definition includes fetch_utxos_deps
# Search for the UtxoStore trait definition
rg -n -A 20 'trait UtxoStore' --type rustLength of output: 0
🏁 Script executed:
#!/bin/bash
# Description: Find imports and usage of UtxoStore in the target file
# First, show the imports and struct/impl context in utxos.rs
echo "=== FILE: crates/trp/src/utxos.rs (beginning) ==="
head -n 110 crates/trp/src/utxos.rs
echo -e "\n=== SEARCH: UtxoStore references ==="
rg -n 'UtxoStore' crates/trp/src/utxos.rs -B 2 -A 2
echo -e "\n=== SEARCH: impl blocks in utxos.rs ==="
rg -n '^impl' crates/trp/src/utxos.rs -A 5Length of output: 3936
🏁 Script executed:
#!/bin/bash
# Description: Check tx3_lang dependency version and search for compilation context
echo "=== SEARCH: Cargo.toml for tx3_lang dependency ==="
rg -n 'tx3_lang' crates/trp/Cargo.toml -B 1 -A 1
echo -e "\n=== SEARCH: Cargo.lock or dependency resolution ==="
fd -n 'Cargo.lock' . -exec grep -A 10 'name = "tx3_lang"' {} \;
echo -e "\n=== SEARCH: Error or diagnostic comments in utxos.rs ==="
rg -n 'TODO|FIXME|XXX|ERROR|WARN' crates/trp/src/utxos.rs
echo -e "\n=== SEARCH: All methods in UtxoStore impl ==="
rg -n 'async fn|fn ' crates/trp/src/utxos.rs | grep -A 1 -B 1 'fetch\|narrow'Length of output: 633
🏁 Script executed:
#!/bin/bash
# Description: Find Cargo.toml files and tx3_lang dependency info
echo "=== SEARCH: All Cargo.toml files ==="
fd 'Cargo.toml' . -type f
echo -e "\n=== ROOT Cargo.toml for tx3_lang ==="
rg -n 'tx3_lang' Cargo.toml -B 1 -A 1 2>/dev/null || echo "Not found in root"
echo -e "\n=== SEARCH: Cargo.lock for tx3_lang ==="
if [ -f Cargo.lock ]; then
grep -A 5 'name = "tx3_lang"' Cargo.lock || echo "tx3_lang not in Cargo.lock"
else
echo "No Cargo.lock found"
fi
echo -e "\n=== CONTEXT: Is fetch_utxos_deps the only questionable method? ==="
sed -n '48,110p' crates/trp/src/utxos.rs | head -65Length of output: 2290
🏁 Script executed:
#!/bin/bash
# Description: Find tx3_lang crate and check UtxoStore trait definition
echo "=== SEARCH: Find tx3-lang or tx3_lang directories ==="
find . -maxdepth 2 -type d -name '*tx3*' 2>/dev/null
echo -e "\n=== SEARCH: Find Cargo.toml files that define tx3_lang ==="
rg -l 'name = "tx3' .
echo -e "\n=== SEARCH: UtxoStore trait definition ==="
rg -n 'trait UtxoStore' . -A 30 --type rust
echo -e "\n=== SEARCH: fetch_utxos_deps in any file ==="
rg -n 'fetch_utxos_deps' . -B 2 -A 2 --type rustLength of output: 445
🏁 Script executed:
#!/bin/bash
# Description: Extract tx3_lang version and check .expect() usage safety
echo "=== EXTRACT: tx3_lang version from Cargo.lock ==="
grep -A 3 'name = "tx3_lang"' Cargo.lock | head -6
echo -e "\n=== ANALYZE: Line 93 with .expect() ==="
sed -n '85,100p' crates/trp/src/utxos.rs
echo -e "\n=== SEARCH: How Era::try_from is used elsewhere ==="
rg -n 'Era::try_from' . --type rust -B 2 -A 2
echo -e "\n=== SEARCH: Error handling patterns in this file ==="
rg -n '\.expect\(|\.unwrap\(\)' crates/trp/src/utxos.rsLength of output: 2900
Replace .expect() with proper error handling at line 93.
The .expect() will panic if Era::try_from(*c) fails. Other parts of the codebase handle this conversion with the ? operator (e.g., crates/trp/src/mapping.rs:117). Use error propagation instead:
let era = pallas::ledger::traverse::Era::try_from(*c)?;This allows the function to return the conversion error rather than panicking at runtime.
🧰 Tools
🪛 GitHub Actions: CI
[error] 80-80: method fetch_utxos_deps is not a member of trait UtxoStore.
🪛 GitHub Check: Check Build
[failure] 80-80:
method fetch_utxos_deps is not a member of trait UtxoStore
🤖 Prompt for AI Agents
In crates/trp/src/utxos.rs around lines 80 to 103, the closure uses
Era::try_from(...).expect(...) which will panic on conversion failure; replace
the panic with proper error propagation by using the ? operator (or map_err to
convert the conversion error into the function's Result error type) so the
function returns an Err instead of panicking. Modify the mapping to propagate
the conversion error (e.g., use Era::try_from(*c)? or
Era::try_from(*c).map_err(|e| Error::from(e)) as needed) and adjust the closure
or surrounding code to return a Result that can be collected into the final
UtxoMap, ensuring types align with tx3_lang::backend::Error.
| .map(|(txoref, eracbor)| { | ||
| let TxoRef(a, b) = txoref; | ||
| let EraCbor(c, d) = eracbor.as_ref(); | ||
| let era = pallas::ledger::traverse::Era::try_from(*c).expect("era out of range"); |
There was a problem hiding this comment.
Replace panic with proper error handling.
Using .expect() will panic at runtime if the era conversion fails. This is particularly risky for production code handling external/untrusted data.
Apply this diff to handle the error gracefully:
- let era = pallas::ledger::traverse::Era::try_from(*c).expect("era out of range");
+ let era = pallas::ledger::traverse::Era::try_from(*c)
+ .map_err(|_| tx3_lang::backend::Error::Storage(format!("Invalid era value: {}", c)))?;Note: You'll need to refactor the closure in the map call (lines 90-99) to return a Result and use collect::<Result<_, _>>() on line 100, similar to the pattern in fetch_utxos (lines 72-75).
Committable suggestion skipped: line range outside the PR's diff.
1 participant
Summary by CodeRabbit
Refactor
New Features
Chores