feat!: partial implementation of builtin liquidity pool - #1628
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRenames GetResourceType→GetResourceInfo and exposes ResourceInfo (resource_type + divisibility); replaces amount() semantics with unlocked_amount()/is_empty(), adds BucketAction::DropEmpty and BucketGetAmountArg with automatic post-take cleanup, adds Amount arithmetic helpers, implements a two-resource liquidity pool, and bumps workspace/version and features. Changes
Sequence Diagram(s)sequenceDiagram
participant Processor
participant Engine
participant BucketActor
Processor->>Engine: Invoke BucketAction::Take(bucket_id, amount)
Engine->>BucketActor: Perform take, then Invoke GetAmount(BucketGetAmountArg::Everything)
BucketActor-->>Engine: Return Amount (unlocked_amount)
alt unlocked_amount == 0
Engine->>BucketActor: Invoke BucketAction::DropEmpty(bucket_id)
BucketActor-->>Engine: Confirm drop
Engine-->>Processor: Return success (bucket dropped)
else unlocked_amount > 0
Engine-->>Processor: Return success (bucket still exists)
end
sequenceDiagram
participant User
participant LiquidityPool
participant VaultA
participant VaultB
participant LPToken
User->>LiquidityPool: contribute(bucket_a, bucket_b)
LiquidityPool->>LiquidityPool: validate, compute lp_mint (sqrt/proportional)
LiquidityPool->>LPToken: mint(lp_mint)
LiquidityPool->>VaultA: deposit(contribution_a)
LiquidityPool->>VaultB: deposit(contribution_b)
LiquidityPool-->>User: return lp_bucket
User->>LiquidityPool: redeem(lp_bucket)
LiquidityPool->>LiquidityPool: compute redemption amounts
LiquidityPool->>VaultA: withdraw(amount_a)
LiquidityPool->>VaultB: withdraw(amount_b)
LiquidityPool->>LPToken: burn(lp_bucket)
LiquidityPool-->>User: return (bucket_a, bucket_b)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/engine/src/runtime/impl.rs(5 hunks)crates/engine/src/runtime/working_state.rs(2 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/events.rs(2 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/events.rs(1 hunks)crates/engine_types/src/resource.rs(1 hunks)crates/engine_types/src/resource_container.rs(6 hunks)crates/template_builtin/templates/pool/Cargo.toml(1 hunks)crates/template_builtin/templates/pool/src/lib.rs(1 hunks)crates/template_lib/src/args/types.rs(2 hunks)crates/template_lib/src/models/bucket.rs(1 hunks)crates/template_lib/src/models/metadata.rs(2 hunks)crates/template_lib/src/resource/manager.rs(2 hunks)crates/template_lib_types/src/amount/amount.rs(3 hunks)crates/template_lib_types/src/resource_type.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
crates/template_lib/src/models/bucket.rs (2)
crates/template_lib/src/resource/manager.rs (1)
resp(144-144)crates/template_lib/src/models/proof.rs (3)
resp(132-132)resp(175-175)resp(195-195)
crates/engine_types/src/bucket.rs (1)
crates/engine_types/src/resource_container.rs (1)
is_empty(866-868)
crates/engine_types/src/resource_container.rs (2)
crates/template_lib/src/resource/builder/mod.rs (1)
public_fungible(54-56)crates/engine_types/src/bucket.rs (2)
amount(55-57)is_empty(51-53)
crates/engine/src/runtime/working_state.rs (2)
crates/engine_types/src/resource_container.rs (3)
public_fungible(55-63)resource_address(175-182)amount(134-141)crates/engine_types/src/bucket.rs (2)
resource_address(67-69)amount(55-57)
crates/template_lib/src/models/metadata.rs (2)
crates/engine_types/src/indexed_value.rs (2)
metadata(105-107)metadata(313-315)crates/engine_types/src/resource.rs (1)
metadata(203-205)
crates/template_lib_types/src/amount/amount.rs (1)
crates/template_lib/src/models/stealth.rs (1)
new(69-80)
crates/engine/src/transaction/processor.rs (5)
bindings/src/types/Amount.ts (1)
Amount(12-12)crates/engine_types/src/bucket.rs (1)
amount(55-57)crates/engine_types/src/resource_container.rs (1)
amount(134-141)crates/template_lib/src/models/bucket.rs (1)
amount(187-195)crates/template_lib/src/models/proof.rs (1)
amount(104-112)
crates/template_lib/src/resource/manager.rs (7)
bindings/src/types/StealthValueProof.ts (1)
StealthValueProof(9-15)crates/engine_types/src/resource.rs (2)
resource_type(107-109)divisibility(211-213)crates/engine_types/src/resource_container.rs (1)
resource_type(184-191)crates/template_lib/src/models/bucket.rs (1)
resource_type(88-97)crates/template_lib/src/models/vault.rs (1)
resource_type(364-366)crates/template_lib/src/models/proof.rs (4)
resource_type(82-91)resp(132-132)resp(175-175)resp(195-195)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/template_builtin/templates/pool/src/lib.rs (3)
crates/template_lib/src/resource/manager.rs (4)
get(104-106)from(993-995)resource_address(109-111)resource_type(119-121)crates/template_lib/src/models/bucket.rs (4)
from(50-52)resource_address(76-85)amount(187-195)resource_type(88-97)crates/engine/src/runtime/impl.rs (1)
emit_event(435-458)
crates/engine/src/runtime/impl.rs (4)
crates/engine_types/src/resource.rs (2)
divisibility(211-213)resource_type(107-109)crates/engine_types/src/resource_container.rs (3)
resource_type(184-191)public_fungible(55-63)resource_address(175-182)crates/template_lib/src/models/bucket.rs (2)
resource_type(88-97)resource_address(76-85)crates/template_lib/src/args/types.rs (3)
resource_address(448-453)resource_address(645-650)bucket_id(455-460)
crates/engine/tests/events.rs (2)
crates/engine_types/src/events.rs (1)
payload(119-121)crates/template_lib/src/models/metadata.rs (1)
get(49-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: file licenses
- GitHub Check: clippy
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: check nightly
- GitHub Check: test
🔇 Additional comments (16)
crates/template_builtin/templates/pool/Cargo.toml (1)
1-18: Well-structured manifest for liquidity pool template crate.The Cargo.toml is properly configured for a compiled template with appropriate defaults:
- Dual crate-type (cdylib + lib) is correct for template compilation and library usage
- Dependency on tari_template_lib with minimal feature set (macro, alloc) aligns with embedded/low-footprint design
- Release profile settings (opt-level='s', lto=true, codegen-units=1, panic='abort', strip='debuginfo') are appropriate for optimizing compiled template size and performance
Please verify that the relative path dependency
../../../../crates/template_libcorrectly resolves to the intended location in your monorepo structure, and confirm that the "macro" and "alloc" features are consistent with how other template crates in the project declare tari_template_lib dependencies.crates/engine_types/src/resource.rs (1)
207-209: The change is correct and improves the code.The original comment assumes that removing
.map(|s| s.as_str())would break the API, but this is incorrect. TheMetadata::get()method already returnsOption<&str>(it internally applies.map()to convert fromOption<&String>toOption<&str>). The original code intoken_symbol()was applying a redundant.map()on an already-unwrapped type. All existing callers handleOption<&str>correctly, including those that further convert to owned strings or use the value directly.crates/engine_types/src/events.rs (1)
115-117: LGTM! Simplified payload access aligns with updated Metadata API.The change correctly removes the redundant
.map(|s| s.as_str())sinceMetadata::getnow returnsOption<&str>directly instead ofOption<&String>.crates/engine/tests/events.rs (1)
118-119: LGTM! Test assertions correctly updated for new Metadata API.The removal of dereference operators aligns with
Metadata::getnow returningOption<&str>directly instead ofOption<&String>.Also applies to: 131-135
crates/template_lib/src/models/metadata.rs (4)
49-51: LGTM! Improved ergonomics with direct &str return.Changing from
Option<&String>toOption<&str>reduces unnecessary indirection and aligns with common Rust patterns for string accessors.
53-56: LGTM! Useful addition following standard library patterns.The
get_or_insertmethod follows theBTreeMap::entry().or_insert()pattern, providing a convenient way to retrieve or create default values.
58-60: LGTM! Completes the mutation API surface.The
removemethod provides necessary functionality for metadata cleanup operations, returning the removed value for potential reuse.
182-186: LGTM! Test expectations correctly updated.Test assertions properly reflect the new
Option<&str>return type fromget().crates/engine_types/src/resource_container.rs (2)
55-63: LGTM! Constructor rename clarifies resource visibility.Renaming from
fungibletopublic_fungiblehelps distinguish from confidential and stealth fungible resources, improving API clarity.
866-868: LGTM! Useful emptiness check for resource containers.The
is_empty()method correctly checks both revealed amounts and confidential commitments, providing a comprehensive emptiness indicator.crates/engine_types/src/bucket.rs (1)
51-53: LGTM! Properly exposes resource container emptiness check.The method correctly delegates to the underlying
ResourceContainer::is_empty(), maintaining consistency with other bucket delegation patterns.crates/template_lib/src/models/bucket.rs (1)
172-182: LGTM! Well-documented bucket cleanup method.The
drop_empty()method follows established patterns for bucket operations and clearly documents that it will panic if the bucket is not empty. This provides an explicit cleanup path for empty buckets.crates/engine/src/runtime/working_state.rs (2)
595-595: LGTM! Consistent use of renamed constructor.The change from
fungibletopublic_fungiblealigns with the resource container API update.
400-405: Validation change is intentional and sound; templates are compatible.The stricter bucket validation (checking
!self.buckets.is_empty()instead of checking only positive amounts) is a design improvement that enforces better transaction accounting. Thedrop_empty()API provides an escape hatch for explicit empty bucket handling, though it's not called anywhere in the codebase because well-formed templates already consume all buckets completely (viadeposit(),burn(), orjoin()).Manifest examples like
tariswap_do_swap.rsdemonstrate the correct pattern: buckets are withdrawn, passed through operations, and immediately deposited—leaving no dangling buckets. The stricter validation prevents accidentally leaving buckets unconsumed, which is a positive change. No template updates are required since the existing codebase already follows sound bucket lifecycle practices.crates/template_lib/src/args/types.rs (2)
232-233: LGTM! Enum variant rename reflects expanded information.Renaming
GetResourceTypetoGetResourceInfoindicates the action now returns additional resource metadata (such as divisibility) beyond just the type, improving the API's expressiveness.
486-486: LGTM! New action supports explicit bucket cleanup.The
DropEmptyvariant enables the newBucket::drop_empty()functionality, providing a structured way to dispose of empty buckets.
ad9abef to
a2b6c85
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
crates/engine/src/runtime/impl.rs (1)
2133-2151: DropEmpty consumes non-empty buckets before erroring—critical data loss.This implementation calls
take_bucketimmediately, removing the bucket from state. If the bucket is non-empty, an error is returned, but the bucket and its contents have already been removed and are silently lost forever. This is the exact issue flagged in the previous review and remains unfixed.Apply this diff to check emptiness before removing the bucket:
BucketAction::DropEmpty => { let bucket_id = bucket_ref.bucket_id().ok_or_else(|| RuntimeError::InvalidArgument { argument: "bucket_ref", reason: "DropEmpty bucket action requires a bucket id".to_string(), })?; args.assert_no_args("Bucket::DropEmpty")?; self.tracker.write_with(|state| { - let bucket = state.take_bucket(bucket_id)?; - if !bucket.is_empty() { - return Err(RuntimeError::InvalidArgument { - argument: "bucket_ref", - reason: "Cannot drop a non-empty bucket".to_string(), - }); - } - // Drop - Ok(InvokeResult::unit()) + { + let bucket = state.get_bucket(bucket_id)?; + if !bucket.is_empty() { + return Err(RuntimeError::InvalidArgument { + argument: "bucket_ref", + reason: "Cannot drop a non-empty bucket".to_string(), + }); + } + } + let _ = state.take_bucket(bucket_id)?; + Ok(InvokeResult::unit()) }) },crates/template_builtin/templates/pool/src/lib.rs (3)
147-172: LP minting breaks the invariant—allows manipulation.The implementation deposits funds before reading vault balances (lines 160-161), then uses post-deposit balances to calculate ratios. This breaks the proportionality invariant and allows contributors to mint arbitrary LP tokens by depositing unbalanced amounts. Additionally, the formula
a_ratio * a_amount + b_ratio * b_amount(line 168) incorrectly adds quantities that represent different resources.This is the exact issue flagged in the previous review and remains unfixed.
Apply this diff to capture pre-deposit balances and mint proportionally to the tighter leg:
pub fn contribute(&mut self, bucket_a: Bucket, bucket_b: Bucket) -> Bucket { // check that the buckets are correct let resource_a = bucket_a.resource_address(); let resource_b = bucket_b.resource_address(); self.assert_pool_resource(resource_a); self.assert_pool_resource(resource_b); assert_ne!(resource_a, resource_b, "The resources must be different"); // extract the bucket amounts for later let a_amount = bucket_a.amount(); let b_amount = bucket_b.amount(); - // add the liquidity to the pool - self.vault_a.deposit(bucket_a); - self.vault_b.deposit(bucket_b); - - // get the bucket/pool ratios - let a_ratio = self.get_a_ratio(a_amount); - let b_ratio = self.get_b_ratio(b_amount); - - // the amount of new lp tokens are proportional to the bucket-pool ratios - let new_lp_amount = a_ratio * a_amount + b_ratio * b_amount; - - // mint and return the new lp tokens - self.lp_resource.mint_fungible(new_lp_amount) + let current_a = self.vault_a.balance(); + let current_b = self.vault_b.balance(); + let lp_supply = self.lp_total_supply(); + + let mint_amount = if lp_supply.is_zero() { + assert!(!a_amount.is_zero() && !b_amount.is_zero(), "Cannot bootstrap the pool with a zero amount"); + // For initial deposit, mint based on the smaller amount to establish price + if a_amount < b_amount { a_amount } else { b_amount } + } else { + // Mint proportionally to share added, using the tighter constraint + let minted_from_a = (lp_supply * a_amount).checked_div(current_a).expect("Division by current_a"); + let minted_from_b = (lp_supply * b_amount).checked_div(current_b).expect("Division by current_b"); + if minted_from_a < minted_from_b { minted_from_a } else { minted_from_b } + }; + assert!(!mint_amount.is_zero(), "Liquidity contribution mints zero LP tokens"); + + self.vault_a.deposit(bucket_a); + self.vault_b.deposit(bucket_b); + self.lp_resource.mint_fungible(mint_amount) }
174-195: Redeem returns zero for partial withdrawals—precision bug.The scaled-division pattern at lines 182-187 multiplies both numerator and denominator by the same
decimalsfactor, which cancels out and provides no precision benefit. Whenlp_amount < total_supply, the integer division yieldslp_ratio = 0, so redeemers receive zero tokens. This renders partial redemptions completely broken.This is the exact issue flagged in the previous review and remains unfixed.
Apply this diff to compute the share directly:
pub fn redeem(&mut self, lp_bucket: Bucket) -> (Bucket, Bucket) { let lp_amount = lp_bucket.amount(); // get the pool information let a_balance = self.vault_a.balance(); let b_balance = self.vault_b.balance(); - // calculate the amount of tokens to take from each pool - let decimals = Amount::from(1_000_000u64); - let lp_ratio = (lp_amount * decimals) / (self.lp_total_supply() * decimals); - - // TODO: div_ceil is probably not a great rounding function - let a_amount = (lp_ratio.div_ceil(decimals)) * a_balance; - let b_amount = (lp_ratio.div_ceil(decimals)) * b_balance; + let total_supply = self.lp_total_supply(); + assert!(!total_supply.is_zero(), "Cannot redeem liquidity when LP supply is zero"); + + let a_amount = (a_balance * lp_amount).checked_div(total_supply).expect("Division by total_supply in redeem"); + let b_amount = (b_balance * lp_amount).checked_div(total_supply).expect("Division by total_supply in redeem"); // burn the LP tokens lp_bucket.burn(); // return the pool tokens let a_bucket = self.vault_a.withdraw(a_amount); let b_bucket = self.vault_b.withdraw(b_amount); (a_bucket, b_bucket) }
220-233: Redemption preview mirrors the same precision bug.
get_redemption_valuerepeats the scaled-division pattern fromredeem, so it also reports zero for fractional LP amounts. Ifredeemis fixed but this preview function is not, they will report inconsistent values.This is the exact issue flagged in the previous review and remains unfixed.
Apply this diff to match the corrected redemption calculation:
pub fn get_redemption_value(&self, lp_amount: Amount) -> (Amount, Amount) { // get the pool information let a_balance = self.vault_a.balance(); - let a_decimals = self.vault_a.to_resource_manager().divisibility(); let b_balance = self.vault_b.balance(); - let b_decimals = self.vault_b.to_resource_manager().divisibility(); - // calculate the amount of tokens to take from each pool - let decimals = Amount::TEN.pow(u8::max(a_decimals, b_decimals) as u32); - let lp_ratio = (lp_amount * decimals) / (self.lp_total_supply() * decimals); - let a_amount = (lp_ratio.div_ceil(decimals)) * a_balance; - let b_amount = (lp_ratio.div_ceil(decimals)) * b_balance; + let total_supply = self.lp_total_supply(); + if total_supply.is_zero() { + return (Amount::zero(), Amount::zero()); + } + + let a_amount = (a_balance * lp_amount).checked_div(total_supply).expect("Division by total_supply in get_redemption_value"); + let b_amount = (b_balance * lp_amount).checked_div(total_supply).expect("Division by total_supply in get_redemption_value"); (a_amount, b_amount) }
🧹 Nitpick comments (8)
crates/template_lib/src/models/metadata.rs (2)
53-56: Add test coverage for the newget_or_insertmethod.The implementation looks correct, but this new public API method lacks test coverage. Consider adding tests to verify:
- Inserting a new key-value pair and returning the value
- Returning an existing value without modifying the map
58-60: Add test coverage for the newremovemethod.This new public API method lacks test coverage. Consider adding tests to verify:
- Removing an existing key returns Some(value)
- Removing a non-existent key returns None
crates/template_lib/src/resource/manager.rs (1)
113-121: Consider: Resource type accessor now makes additional engine call.The
resource_type()method now delegates toresource_info(), which queries bothresource_typeanddivisibilityfrom the engine. While this is cleaner architecturally, it means:
- Callers who only need the type still fetch divisibility
- Repeated calls to
resource_type()make multiple engine calls (no caching)The inline NOTE is helpful, but consider whether frequently-called code paths might see a performance impact.
crates/engine/src/runtime/impl.rs (1)
866-888: Update error message to reflect new action name.The implementation correctly returns
ResourceInfowith bothresource_typeanddivisibility. However, the error message at line 872 still references the old action name.Apply this diff:
let resource_address = resource_ref .as_resource_address() .ok_or_else(|| RuntimeError::InvalidArgument { argument: "resource_ref", - reason: "GetResourceType resource action requires a resource address".to_string(), + reason: "GetResourceInfo resource action requires a resource address".to_string(), })?;crates/template_builtin/templates/pool/src/lib.rs (4)
68-69: Consider higher divisibility for LP tokens.LP tokens are created with
divisibility(0), meaning they cannot be fractionally owned. This severely limits precision for liquidity providers and may cause rounding issues in contribute/redeem calculations. Standard practice is to use higher divisibility (e.g., 6-18) to enable fine-grained ownership and better price discovery.Consider changing:
let lp_resource = ResourceBuilder::public_fungible() - .with_divisibility(0) + .with_divisibility(18) // or another appropriate value
197-208: Clarify add_liquidity vs contribute semantics.The
add_liquiditymethod allows depositing a single asset without minting LP tokens, which can break the pool's constant product invariant. This appears to be an administrative function, but the naming overlaps withcontribute(which mints LP tokens for balanced adds). Consider:
- Renaming to
admin_add_liquidityor similar to clarify intent- Documenting when this should be used vs.
contribute- Adding a check that the pool maintains reasonable balance ratios after single-sided adds
210-218: Clarify remove_liquidity vs redeem semantics.Similar to
add_liquidity, theremove_liquiditymethod allows withdrawing assets without burning LP tokens, which breaks the proportionality invariant. This differs fromredeem, which properly burns LP tokens. The same concerns apply: rename for clarity, document usage, and ensure the pool maintains valid state.
264-282: Inconsistent zero checks between get_a_ratio and get_b_ratio.
get_a_ratio(line 267) usesbalance == 0whileget_b_ratio(line 277) correctly usesbalance.is_zero(). For consistency and to follow Rust best practices with custom types, both should use theis_zero()method.Apply this diff:
pub fn get_a_ratio(&self, amount: Amount) -> Amount { let balance = self.vault_a.balance(); - if balance == 0 { + if balance.is_zero() { Amount::ONE } else { amount.checked_div(balance).expect("Division by zero in get_a_ratio") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
crates/engine/src/runtime/impl.rs(5 hunks)crates/engine/src/runtime/working_state.rs(2 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/events.rs(2 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/events.rs(1 hunks)crates/engine_types/src/resource.rs(1 hunks)crates/engine_types/src/resource_container.rs(6 hunks)crates/template_builtin/templates/pool/Cargo.toml(1 hunks)crates/template_builtin/templates/pool/src/lib.rs(1 hunks)crates/template_lib/src/args/types.rs(2 hunks)crates/template_lib/src/models/bucket.rs(1 hunks)crates/template_lib/src/models/metadata.rs(2 hunks)crates/template_lib/src/resource/manager.rs(2 hunks)crates/template_lib_types/src/amount/amount.rs(3 hunks)crates/template_lib_types/src/resource_type.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/template_builtin/templates/pool/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/template_lib/src/args/types.rs
- crates/template_lib/src/models/bucket.rs
- crates/engine_types/src/resource_container.rs
- crates/engine_types/src/events.rs
🧰 Additional context used
🧬 Code graph analysis (10)
crates/engine_types/src/bucket.rs (1)
crates/engine_types/src/resource_container.rs (1)
is_empty(866-868)
crates/engine/src/runtime/impl.rs (4)
crates/engine_types/src/resource.rs (2)
divisibility(211-213)resource_type(107-109)crates/template_lib/src/resource/manager.rs (3)
divisibility(129-131)resource_type(119-121)resource_address(109-111)crates/engine_types/src/resource_container.rs (3)
resource_type(184-191)public_fungible(55-63)resource_address(175-182)crates/template_lib/src/args/types.rs (3)
resource_address(448-453)resource_address(645-650)bucket_id(455-460)
crates/engine/src/transaction/processor.rs (5)
bindings/src/types/Amount.ts (1)
Amount(12-12)crates/engine_types/src/bucket.rs (1)
amount(55-57)crates/engine_types/src/resource_container.rs (1)
amount(134-141)crates/template_lib/src/models/bucket.rs (1)
amount(187-195)crates/template_lib/src/models/proof.rs (1)
amount(104-112)
crates/template_lib_types/src/resource_type.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/engine/src/runtime/working_state.rs (2)
crates/engine_types/src/resource_container.rs (3)
public_fungible(55-63)resource_address(175-182)amount(134-141)crates/engine_types/src/bucket.rs (2)
resource_address(67-69)amount(55-57)
crates/template_lib/src/models/metadata.rs (2)
crates/engine_types/src/indexed_value.rs (2)
metadata(105-107)metadata(313-315)crates/engine_types/src/resource.rs (1)
metadata(203-205)
crates/engine/tests/events.rs (2)
crates/engine_types/src/events.rs (1)
payload(119-121)crates/template_lib/src/models/metadata.rs (1)
get(49-51)
crates/template_lib_types/src/amount/amount.rs (2)
crates/template_lib/src/models/stealth.rs (1)
new(69-80)crates/common_types/src/vote_power.rs (3)
sub(73-75)mul(81-83)div(89-91)
crates/template_builtin/templates/pool/src/lib.rs (5)
crates/template_lib/src/models/metadata.rs (5)
new(38-40)get(49-51)from(92-94)from(98-100)from(104-106)crates/template_lib_types/src/amount/amount.rs (2)
new(52-54)is_zero(62-64)crates/engine_types/src/resource_container.rs (4)
public_fungible(55-63)resource_address(175-182)amount(134-141)resource_type(184-191)crates/template_lib/src/resource/manager.rs (4)
get(104-106)from(993-995)resource_address(109-111)resource_type(119-121)crates/engine/src/runtime/impl.rs (1)
emit_event(435-458)
crates/template_lib/src/resource/manager.rs (5)
crates/engine_types/src/resource.rs (2)
resource_type(107-109)divisibility(211-213)crates/engine_types/src/resource_container.rs (1)
resource_type(184-191)crates/template_lib/src/models/bucket.rs (1)
resource_type(88-97)crates/template_lib/src/models/vault.rs (1)
resource_type(364-366)crates/template_lib/src/models/proof.rs (4)
resource_type(82-91)resp(132-132)resp(175-175)resp(195-195)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: clippy
- GitHub Check: machete
🔇 Additional comments (22)
crates/template_lib/src/models/metadata.rs (2)
49-51: LGTM! More idiomatic return type.Returning
Option<&str>instead ofOption<&String>is more idiomatic and flexible for callers. The implementation correctly usesmap(|s| s.as_str())to convert the reference.
182-185: LGTM! Tests correctly updated.The test assertions correctly expect
&strvalues, aligning with the updatedgetmethod signature.crates/template_lib_types/src/amount/amount.rs (3)
7-7: LGTM: Import cleanup aligns with macro expansion.Removing
ops::Negfrom the import list is appropriate since thenewtype_ops!macro now generates theNegimplementation automatically.
297-305: LGTM: Exponentiation methods follow existing patterns.The
powandpow_checkedmethods are consistent with other arithmetic operations onAmount. The checked variant properly handles overflow by returningOption<Self>.
394-395: LGTM: Extended arithmetic operations.Adding
remandnegto thenewtype_ops!macro provides complete arithmetic functionality forAmount. This is a non-breaking public API extension.crates/engine_types/src/resource.rs (1)
207-209: LGTM: Simplified token symbol accessor.Removing the redundant
.map(|s| s.as_str())is correct sincemetadata.get()already returnsOption<&str>directly.crates/engine_types/src/bucket.rs (1)
51-53: LGTM: Emptiness check delegation.The
is_empty()method properly delegates to the underlyingresource_container, consistent with other accessor methods in this struct.crates/engine/tests/events.rs (2)
118-119: LGTM: Removed unnecessary dereferences.The assertion is cleaner without the dereference operators since
payload().get()already returns&str(when unwrapped fromOption<&str>).
131-135: LGTM: Consistent dereference cleanup.Same improvement as the previous assertion block—removing unnecessary dereferences for cleaner test code.
crates/engine/src/runtime/working_state.rs (2)
595-595: LGTM: Renamed topublic_fungiblefor clarity.The rename from
fungibletopublic_fungiblealigns with theResourceType::is_public_fungible()naming convention, improving API consistency.
400-402: Validation logic is correct—no action required.Based on verification, the stricter bucket validation (treating any remaining bucket as dangling) is the intended design and works correctly:
Empty buckets ARE dropped: After
Takeoperations inprocessor.rs(line 341), when a bucket becomes zero-valued,BucketAction::DropEmptyis explicitly invoked.DropEmpty removes from state: The
DropEmptyaction callstake_bucket(), which removes the bucket from theHashMapentirely via.remove(&bucket_id)(working_state.rs line 482).Validation expectation is sound: At finalization, the check
if !self.buckets.is_empty()correctly expects zero buckets—any remaining bucket indicates a logic error, not just an empty one.The code follows the intended behavior: empty buckets are cleaned up before finalization, so the validation can safely treat any remaining bucket as an error condition.
crates/engine/src/transaction/processor.rs (2)
45-45: LGTM: Added required import for Amount.The
Amountimport is necessary for decoding theGetAmountresult in the new post-take cleanup logic.
328-342: Review: Post-take empty bucket cleanup.The cleanup logic automatically drops empty buckets after
Takeoperations to prevent dangling buckets at finalization. The flow is:
- Invoke
Takeon the bucket- Query remaining amount via
GetAmount- If zero, invoke
DropEmptyMinor concern: The code uses
.decode::<Amount>()?which will propagate errors, but there's no explicit error handling if the decode fails. Consider whether a more descriptive error message would help debugging if this fails unexpectedly.However, the logic itself appears sound and aligns with the stricter bucket validation in
working_state.rs.crates/template_lib/src/resource/manager.rs (3)
46-46: LGTM: Added ResourceInfo import.The new
ResourceInfotype is needed for theresource_info()method introduced below.
123-131: LGTM: Divisibility accessor added.The new
divisibility()method complements the existing API and follows the same pattern asresource_type().
133-145: LGTM: Centralized resource info query.The
resource_info()method is the primary accessor for resource metadata, returning both type and divisibility in a single engine call. This is more efficient than separate queries.crates/template_lib_types/src/resource_type.rs (3)
4-4: LGTM: Import simplification.Removing
FromStrfrom the top-level imports is appropriate since it's now conditionally imported within thestd-gatedparsingmodule.
64-96: LGTM: Feature-gated parsing module with proper re-export.The
FromStrimplementation andParseResourceTypeErrorare correctly placed in astd-gated module, and the error type is properly re-exported at line 96, resolving the concern from the previous review.
98-102: LGTM: ResourceInfo struct added.The new
ResourceInfostruct provides a unified way to query both resource type and divisibility, supporting the updatedGetResourceInfoengine action. Public fields are appropriate for this simple data structure.crates/engine/src/runtime/impl.rs (2)
120-120: LGTM: ResourceInfo import added.The import is necessary for the new
GetResourceInfoaction that returns both resource type and divisibility.
1383-1383: LGTM: Updated to use public_fungible.The change from
fungibletopublic_fungiblealigns with the broader refactoring to clarify resource container types.crates/template_builtin/templates/pool/src/lib.rs (1)
312-322: LGTM: Resource fungibility validation.The
check_resource_is_fungiblemethod correctly validates that resources are one of the fungible-like types (Fungible, Confidential, or Stealth) and properly rejects NonFungible resources.
a2b6c85 to
43bd29c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/engine/src/runtime/impl.rs (1)
2133-2151: CRITICAL: DropEmpty destroys non-empty buckets before validation.
take_bucketis called on Line 2141 before checking if the bucket is empty. If the bucket contains funds, the error on Line 2143 is returned but the bucket has already been removed from state—the funds are permanently lost.This issue was previously flagged but remains unresolved.
Apply this diff to check emptiness before removal:
BucketAction::DropEmpty => { let bucket_id = bucket_ref.bucket_id().ok_or_else(|| RuntimeError::InvalidArgument { argument: "bucket_ref", reason: "DropEmpty bucket action requires a bucket id".to_string(), })?; args.assert_no_args("Bucket::DropEmpty")?; self.tracker.write_with(|state| { - let bucket = state.take_bucket(bucket_id)?; - if !bucket.is_empty() { - return Err(RuntimeError::InvalidArgument { - argument: "bucket_ref", - reason: "Cannot drop a non-empty bucket".to_string(), - }); - } - // Drop - Ok(InvokeResult::unit()) + { + let bucket = state.get_bucket(bucket_id)?; + if !bucket.is_empty() { + return Err(RuntimeError::InvalidArgument { + argument: "bucket_ref", + reason: "Cannot drop a non-empty bucket".to_string(), + }); + } + } + let _ = state.take_bucket(bucket_id)?; + Ok(InvokeResult::unit()) }) },
🧹 Nitpick comments (1)
crates/engine/src/transaction/processor.rs (1)
327-342: Rename variable for clarity.The variable
prev_bucket_valis misleading—it holds the bucket's amount after the take operation, not before. Consider renaming toremaining_amountorbucket_amount_after_takefor clarity.Apply this diff:
- let prev_bucket_val = runtime + let remaining_amount = runtime .interface() .bucket_invoke(bucket_ref, BucketAction::GetAmount, invoke_args![].into())? .decode::<Amount>()?; - if prev_bucket_val.is_zero() { + if remaining_amount.is_zero() { // Drop the bucket to prevent a dangling (empty) bucket runtime .interface() .bucket_invoke(bucket_ref, BucketAction::DropEmpty, invoke_args![].into())?; }
📜 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 (18)
Cargo.toml(1 hunks)crates/engine/src/runtime/impl.rs(5 hunks)crates/engine/src/runtime/working_state.rs(2 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/tests/events.rs(2 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/events.rs(1 hunks)crates/engine_types/src/resource.rs(1 hunks)crates/engine_types/src/resource_container.rs(6 hunks)crates/template_builtin/templates/pool/Cargo.toml(1 hunks)crates/template_builtin/templates/pool/src/lib.rs(1 hunks)crates/template_lib/src/args/types.rs(2 hunks)crates/template_lib/src/models/bucket.rs(1 hunks)crates/template_lib/src/models/metadata.rs(2 hunks)crates/template_lib/src/resource/manager.rs(2 hunks)crates/template_lib_types/src/amount/amount.rs(3 hunks)crates/template_lib_types/src/resource_type.rs(2 hunks)integration_tests/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/template_lib/src/args/types.rs
- crates/engine_types/src/bucket.rs
- crates/engine_types/src/resource.rs
- crates/engine_types/src/events.rs
- crates/template_builtin/templates/pool/src/lib.rs
- crates/engine/src/runtime/working_state.rs
- crates/template_builtin/templates/pool/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (8)
crates/template_lib/src/models/bucket.rs (2)
crates/template_lib/src/resource/manager.rs (1)
resp(144-144)crates/template_lib/src/models/proof.rs (3)
resp(132-132)resp(175-175)resp(195-195)
crates/template_lib_types/src/resource_type.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/template_lib/src/models/metadata.rs (2)
crates/engine_types/src/indexed_value.rs (2)
metadata(105-107)metadata(313-315)crates/engine_types/src/resource.rs (1)
metadata(203-205)
crates/engine/src/runtime/impl.rs (3)
crates/engine_types/src/resource.rs (2)
divisibility(211-213)resource_type(107-109)crates/engine_types/src/resource_container.rs (3)
resource_type(184-191)public_fungible(55-63)resource_address(175-182)crates/template_lib/src/args/types.rs (3)
resource_address(448-453)resource_address(645-650)bucket_id(455-460)
crates/engine/src/transaction/processor.rs (5)
bindings/src/types/Amount.ts (1)
Amount(12-12)crates/engine_types/src/bucket.rs (1)
amount(55-57)crates/engine_types/src/resource_container.rs (1)
amount(134-141)crates/template_lib/src/models/bucket.rs (1)
amount(187-195)crates/template_lib/src/models/proof.rs (1)
amount(104-112)
crates/engine_types/src/resource_container.rs (2)
crates/template_lib/src/resource/builder/mod.rs (1)
public_fungible(54-56)crates/engine_types/src/bucket.rs (3)
amount(55-57)resource_address(67-69)is_empty(51-53)
crates/template_lib/src/resource/manager.rs (7)
crates/engine_types/src/bucket.rs (1)
resource_type(71-73)crates/engine_types/src/resource.rs (2)
resource_type(107-109)divisibility(211-213)crates/engine_types/src/resource_container.rs (1)
resource_type(184-191)crates/template_lib/src/models/bucket.rs (1)
resource_type(88-97)crates/template_lib/src/models/vault.rs (1)
resource_type(364-366)crates/template_lib/src/models/proof.rs (4)
resource_type(82-91)resp(132-132)resp(175-175)resp(195-195)bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
crates/engine/tests/events.rs (2)
crates/engine_types/src/events.rs (1)
payload(119-121)crates/template_lib/src/models/metadata.rs (1)
get(49-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: machete
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (17)
crates/template_lib/src/models/metadata.rs (4)
49-51: LGTM: Idiomatic API improvement.Changing the return type to
Option<&str>is more idiomatic than returningOption<&String>and provides better flexibility for callers.
53-56: Add test coverage for the new method.The
get_or_insertimplementation is correct and follows Rust conventions. However, there's no test coverage for this new method.Consider adding a test case to verify:
- Insertion behavior when key doesn't exist
- Return of existing value when key exists
- Correct return of
&strreference#[test] fn test_get_or_insert() { let mut metadata = Metadata::new(); // Insert when key doesn't exist let value = metadata.get_or_insert("key1", "default"); assert_eq!(value, "default"); assert_eq!(metadata.get("key1"), Some("default")); // Return existing value when key exists let value = metadata.get_or_insert("key1", "other"); assert_eq!(value, "default"); }
58-60: Add test coverage for the new method.The
removeimplementation is correct. However, there's no test coverage for this new method.Consider adding a test case:
#[test] fn test_remove() { let mut metadata = metadata!("key1" => "value1", "key2" => "value2"); assert_eq!(metadata.remove("key1"), Some("value1".to_string())); assert_eq!(metadata.get("key1"), None); assert_eq!(metadata.remove("nonexistent"), None); }
182-186: LGTM: Tests properly updated.The test assertions correctly validate the new
Option<&str>return type.crates/template_lib_types/src/amount/amount.rs (2)
7-7: LGTM! Import cleanup aligns with macro-generated negation.The removal of
ops::Negfrom imports is consistent with the updatednewtype_ops!macro now generating the negation implementation.
394-395: I need to verify the behavior of thenewtype_opsmacro and confirm edge-case handling inbnum.Now let me search forbnumbehavior with edge cases:Let me verify the exact behavior when
newtype_opsgenerates operators and what trait implementationsbnum::BIntprovides:Perfect! This is the critical finding. Now I have all the information I need.Based on my verification, I can now provide a definitive assessment of the review comment.
Add test coverage for
remandnegoperations, particularly edge cases.The macro-generated operators expose real safety concerns:
Modulus by zero or overflow: The Rem trait will panic if divisor is zero or if
self / otherresults in overflow. ForAmount % Amount::ZEROor negation-induced overflow conditions, this will panic.Division/remainder with MIN/-1: The operations
/, %for arguments INT_MIN and -1 will unconditionally panic. This applies toAmount::MIN % Amount::from(-1).Negation of MIN: Negating MIN causes either undefined behavior (unchecked) or a panic (strict). No tests exist for
(-Amount::MIN).Current state:
- No tests found for the newly added
remoperator- No tests for negation edge cases (e.g.,
Amount::MIN)- Existing negation tests only use normal values, not boundary conditions
Add explicit test cases for:
amount % Amount::zero()(should panic)Amount::MIN % Amount::from(-1)(should panic)-Amount::MIN(should panic or return controlled result)rembehavior with negative operands to document expected sign behaviorcrates/engine/src/transaction/processor.rs (1)
45-45: LGTM – Import addition supports new bucket cleanup logic.The
Amountimport is required for decoding and comparing bucket amounts in the new post-take cleanup flow.crates/template_lib_types/src/resource_type.rs (3)
4-4: LGTM – Import cleanup aligns with module reorganization.Removing the top-level
FromStrimport is correct since it's now only used within theparsingmodule.
64-96: LGTM – Error type is now properly exported.The parsing module correctly encapsulates
FromStrfunctionality and publicly re-exportsParseResourceTypeError, addressing the previous concern about the error type being unreachable.
98-102: LGTM – ResourceInfo provides clean API for resource metadata.The new
ResourceInfostruct appropriately bundlesresource_typeanddivisibility, enabling callers to retrieve both properties in a single engine call.crates/engine/src/runtime/impl.rs (3)
120-120: LGTM – Import supports ResourceInfo return type.
866-887: LGTM – GetResourceInfo consolidates resource metadata retrieval.Returning
ResourceInfoinstead of justResourceTypeallows callers to retrieve bothresource_typeanddivisibilityin a single engine call, improving efficiency.
1383-1383: LGTM – Naming clarifies fungible resource visibility.Using
public_fungibleinstead offungiblemakes the distinction from confidential and stealth resources explicit.crates/template_lib/src/resource/manager.rs (4)
46-46: LGTM – Import enables ResourceInfo usage.
113-121: LGTM – Clean delegation reduces redundant engine calls.Delegating to
resource_info()avoids duplicate logic, and the documentation helpfully suggests callingresource_info()directly when both type and divisibility are needed.
123-131: LGTM – Divisibility accessor complements resource_type().The new
divisibility()method provides a convenient accessor while recommendingresource_info()for callers needing both properties.
133-145: LGTM – Central resource info retrieval method.The
resource_info()method provides a single point for retrieving bothresource_typeanddivisibility, efficiently replacing the priorGetResourceTypecall.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/engine/src/runtime/impl.rs (1)
2133-2150: DropEmpty still deletes non-empty buckets before erroring
We’re still callingtake_bucketup front, so a non-empty bucket is removed from state and its contents are lost before we return the error. Please re-check the bucket first, bail out if it’s non-empty, and only then remove it. This is the same critical issue previously flagged.Apply this patch:
self.tracker.write_with(|state| { - let bucket = state.take_bucket(bucket_id)?; - if !bucket.is_empty() { - return Err(RuntimeError::InvalidArgument { - argument: "bucket_ref", - reason: "Cannot drop a non-empty bucket".to_string(), - }); - } - // Drop - Ok(InvokeResult::unit()) + { + let bucket = state.get_bucket(bucket_id)?; + if !bucket.is_empty() { + return Err(RuntimeError::InvalidArgument { + argument: "bucket_ref", + reason: "Cannot drop a non-empty bucket".to_string(), + }); + } + } + let _ = state.take_bucket(bucket_id)?; + Ok(InvokeResult::unit()) })
🧹 Nitpick comments (3)
crates/template_lib/src/models/account.rs (1)
38-41: LGTM! Clean iterator accessor for vault resource addresses.The implementation correctly delegates to
BTreeMap::keys()and the return type properly captures the iterator semantics. This is a useful addition for enumerating resources in liquidity pool scenarios.Optional: Consider enhancing the documentation to note that iteration order is deterministic (sorted by BTreeMap):
- /// Returns an iterator over all resource addresses in the account. + /// Returns an iterator over all resource addresses in the account. + /// The addresses are yielded in sorted order as determined by the BTreeMap. pub fn all_resources_iter(&self) -> impl Iterator<Item = &ResourceAddress> {crates/template_test_tooling/src/template_test.rs (1)
501-540: Consider documenting the single-vault assumption.The method assumes the faucet component has exactly one vault (Line 533 uses
.first()). If theTestFaucettemplate is modified to include multiple vaults, this could silently return an unexpected resource address.Consider adding a comment or assertion:
let vault_id = indexed .vault_ids() .first() - .expect("No vault id found in faucet component state"); + .expect("TestFaucet component must have exactly one vault");Alternatively, assert that there is exactly one vault:
let vault_ids = indexed.vault_ids(); assert_eq!(vault_ids.len(), 1, "TestFaucet component must have exactly one vault"); let vault_id = &vault_ids[0];crates/template_test_tooling/src/builtin_component_state.rs (1)
82-115: Good simplification.The refactored initialization is cleaner with constants replacing dynamically-derived IDs, and removing the template address parameter simplifies the API.
The comment at Line 96 could be updated for clarity:
- // This must mirror the test faucet component + // This must mirror the XTR faucet component structure
📜 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 (38)
Cargo.toml(2 hunks)applications/tari_indexer/README.md(1 hunks)applications/tari_validator_node/src/genesis_state.rs(2 hunks)crates/engine/src/runtime/fee_state.rs(1 hunks)crates/engine/src/runtime/impl.rs(12 hunks)crates/engine/src/runtime/working_state.rs(9 hunks)crates/engine/src/wasm/module.rs(2 hunks)crates/engine/tests/access_rules.rs(1 hunks)crates/engine/tests/account.rs(3 hunks)crates/engine/tests/composability.rs(2 hunks)crates/engine/tests/fees.rs(4 hunks)crates/engine/tests/test.rs(1 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/commit_result.rs(7 hunks)crates/engine_types/src/proof.rs(1 hunks)crates/engine_types/src/resource_container.rs(12 hunks)crates/engine_types/src/vault.rs(1 hunks)crates/template_builtin/build.rs(1 hunks)crates/template_builtin/src/lib.rs(2 hunks)crates/template_builtin/templates/account/Cargo.toml(1 hunks)crates/template_builtin/templates/account/src/lib.rs(1 hunks)crates/template_builtin/templates/faucet/Cargo.toml(1 hunks)crates/template_builtin/templates/faucet/src/lib.rs(1 hunks)crates/template_builtin/templates/liquidity_pool/Cargo.toml(1 hunks)crates/template_builtin/templates/liquidity_pool/src/lib.rs(1 hunks)crates/template_builtin/tests/liquidity_pool.rs(1 hunks)crates/template_lib/Cargo.toml(1 hunks)crates/template_lib/src/models/account.rs(1 hunks)crates/template_lib/src/models/bucket.rs(1 hunks)crates/template_lib_types/Cargo.toml(2 hunks)crates/template_lib_types/src/amount/amount.rs(6 hunks)crates/template_test_tooling/src/builtin_component_state.rs(2 hunks)crates/template_test_tooling/src/lib.rs(1 hunks)crates/template_test_tooling/src/package_builder.rs(2 hunks)crates/template_test_tooling/src/read_only_state_store.rs(2 hunks)crates/template_test_tooling/src/support/assert_error.rs(2 hunks)crates/template_test_tooling/src/template_test.rs(9 hunks)lints.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/engine/tests/access_rules.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine_types/src/resource_container.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T10:10:24.249Z
Learnt from: sdbondi
Repo: tari-project/tari-ootle PR: 1629
File: applications/tari_walletd/src/handlers/accounts.rs:1001-1002
Timestamp: 2025-11-04T10:10:24.249Z
Learning: In applications/tari_walletd/src/handlers/accounts.rs, the expect() on Memo::new_pay_ref_and_bytes_truncate at line 1002 is safe and intentional. PayRef is validated to be at most 64 bytes during address decoding (PayRef::MAX_LEN = 64), and the function only returns None if payref exceeds 252 bytes (Memo::MAX_BYTES_LENGTH - 1). Since 64 < 252, None is impossible with a valid PayRef.
Applied to files:
crates/engine/tests/fees.rscrates/engine/tests/composability.rs
🧬 Code graph analysis (20)
crates/template_test_tooling/src/lib.rs (1)
crates/template_test_tooling/src/template_test.rs (1)
xtr_faucet_component(71-73)
crates/engine_types/src/commit_result.rs (1)
crates/engine_types/src/fees.rs (1)
is_paid_in_full(58-60)
crates/template_lib/src/models/account.rs (1)
bindings/src/types/ResourceAddress.ts (1)
ResourceAddress(6-6)
crates/engine/src/runtime/fee_state.rs (1)
crates/engine_types/src/proof.rs (2)
amount(42-44)amount(84-86)
crates/engine_types/src/bucket.rs (3)
crates/engine_types/src/resource_container.rs (3)
is_empty(872-876)unlocked_amount(138-145)balance(134-136)crates/engine/src/runtime/engine_args.rs (1)
is_empty(65-67)crates/engine_types/src/vault.rs (1)
balance(105-107)
crates/template_test_tooling/src/read_only_state_store.rs (3)
crates/engine_types/src/events.rs (1)
template_address(107-109)bindings/src/types/ComponentHeader.ts (1)
ComponentHeader(9-17)crates/engine_types/src/substate.rs (1)
component(590-595)
crates/engine/src/runtime/impl.rs (3)
crates/engine_types/src/resource.rs (2)
divisibility(211-213)resource_type(107-109)crates/engine_types/src/resource_container.rs (3)
resource_type(188-195)public_fungible(55-63)resource_address(179-186)crates/template_lib/src/models/bucket.rs (2)
resource_type(88-97)resource_address(76-85)
crates/template_lib/src/models/bucket.rs (2)
crates/engine_types/src/bucket.rs (1)
is_empty(51-53)crates/engine_types/src/resource_container.rs (1)
is_empty(872-876)
crates/template_test_tooling/src/package_builder.rs (2)
crates/template_builtin/src/lib.rs (1)
get_template_builtin(43-45)crates/engine/src/wasm/module.rs (1)
from_code(61-63)
crates/template_builtin/src/lib.rs (1)
crates/template_lib/src/models/component.rs (1)
from_array(67-69)
crates/engine/src/runtime/working_state.rs (4)
bindings/src/types/ResourceContainer.ts (1)
ResourceContainer(11-29)crates/engine_types/src/resource_container.rs (3)
public_fungible(55-63)resource_address(179-186)unlocked_amount(138-145)crates/engine_types/src/bucket.rs (2)
resource_address(72-74)unlocked_amount(55-57)crates/engine_types/src/proof.rs (4)
resource_address(46-48)resource_address(88-90)amount(42-44)amount(84-86)
crates/engine/tests/test.rs (2)
bindings/src/helpers/consts.ts (1)
XTR(10-10)bindings/src/types/Amount.ts (1)
Amount(12-12)
crates/engine/tests/fees.rs (2)
crates/template_test_tooling/src/template_test.rs (1)
xtr_faucet_component(71-73)crates/engine_types/src/fees.rs (1)
total_fees_paid(46-48)
crates/template_builtin/tests/liquidity_pool.rs (2)
bindings/src/helpers/consts.ts (1)
XTR(10-10)crates/template_test_tooling/src/template_test.rs (2)
xtr_faucet_component(71-73)new(98-100)
crates/engine/tests/account.rs (1)
crates/template_test_tooling/src/template_test.rs (1)
xtr_faucet_component(71-73)
crates/template_test_tooling/src/support/assert_error.rs (1)
bindings/src/types/RejectReason.ts (1)
RejectReason(4-12)
applications/tari_validator_node/src/genesis_state.rs (4)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)bindings/src/types/Vault.ts (1)
Vault(5-5)crates/engine_types/src/vault.rs (1)
new(49-54)bindings/src/types/ResourceContainer.ts (1)
ResourceContainer(11-29)
crates/template_test_tooling/src/builtin_component_state.rs (4)
crates/template_test_tooling/src/template_test.rs (2)
test_nft_faucet_component(75-77)xtr_faucet_component(71-73)crates/engine_types/src/resource_container.rs (1)
stealth(91-101)crates/template_lib/src/models/vault.rs (1)
for_test(478-480)bindings/src/types/ComponentHeader.ts (1)
ComponentHeader(9-17)
crates/template_test_tooling/src/template_test.rs (3)
bindings/src/types/IndexedWellKnownTypes.ts (1)
IndexedWellKnownTypes(15-30)crates/template_builtin/src/lib.rs (1)
all_builtin_templates(51-72)crates/template_test_tooling/src/builtin_component_state.rs (1)
initialize_builtin_faucet_state(82-115)
crates/template_builtin/templates/liquidity_pool/src/lib.rs (4)
crates/engine_types/src/resource_container.rs (3)
public_fungible(55-63)resource_address(179-186)resource_type(188-195)crates/engine_types/src/vault.rs (3)
new(49-54)resource_address(121-123)resource_type(125-127)crates/engine/src/runtime/impl.rs (1)
emit_event(435-458)crates/template_lib/src/models/bucket.rs (3)
resource_address(76-85)amount(193-201)resource_type(88-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (46)
crates/template_test_tooling/src/support/assert_error.rs (1)
9-9: Excellent addition of#[track_caller]for better panic diagnostics.Adding
#[track_caller]to these assertion helpers will show the test call site in panic messages rather than the helper's internal panic location, significantly improving the debugging experience when tests fail.Also applies to: 19-19, 26-26
applications/tari_indexer/README.md (1)
56-110: Documentation verification confirms all CLI options are accurate and properly implemented.I verified all documented CLI options against the implementation in
applications/tari_indexer/src/cli.rsand their usage inmain.rs. Every option listed in the README has a corresponding implementation with correct short flags, long flags, and environment variable annotations where applicable. The organization into functional categories (Core Options, Configuration, Logging, API Server, Blockchain Scanning, P2P Networking, Epoch Oracle) accurately reflects the grouping of related CLI parameters. The example usage section demonstrates realistic command patterns that align with the actual CLI structure.lints.toml (1)
73-75: LGTM: Reasonable lint exception with clear justification.The
clippy::drop_non_dropallowance is well-justified for legitimate use cases involving borrow checker constraints.crates/template_builtin/templates/account/src/lib.rs (1)
115-119: LGTM: Efficient early return for empty buckets.The early return avoids unnecessary vault operations when the bucket is empty, improving efficiency and aligning with the new bucket lifecycle handling introduced in this PR.
crates/template_builtin/templates/account/Cargo.toml (1)
11-18: LGTM: Build configuration aligns with builtin template requirements.The switch to
opt-level = 3for builtin templates prioritizes performance over binary size, which is appropriate. The dualcdylibandlibcrate types enable both WASM deployment and integration testing.crates/template_builtin/templates/faucet/Cargo.toml (1)
12-20: LGTM: Consistent build configuration across builtin templates.The build configuration changes match those in the account template, ensuring consistency across all builtin templates in the codebase.
crates/engine_types/src/commit_result.rs (1)
62-143: LGTM: Enhanced panic diagnostics with track_caller.Adding
#[track_caller]to test helper methods improves debugging by showing the actual call site in panic messages rather than the helper method location.Cargo.toml (2)
3-3: LGTM: Version bump reflects breaking changes.The minor version bump from 0.15.1 to 0.16.0 appropriately indicates the breaking changes noted in the PR (ABI changes, data directory deletion required).
213-213: LGTM: Added dependency supports new arithmetic features.The
num-integerdependency enables the newchecked_sqrtfunctionality in the Amount type.crates/engine/src/runtime/working_state.rs (6)
595-595: LGTM: API migration to public_fungible.The change from
fungible()topublic_fungible()aligns with the ResourceContainer API updates across the codebase.
496-498: LGTM: Consistent use of unlocked_amount for balance checks.Using
unlocked_amount()for the zero check is consistent with the API migration throughout this PR.
656-662: LGTM: Supply tracking uses unlocked_amount consistently.The total supply calculations correctly use
unlocked_amount()for both increase and overflow checks.
1336-1399: LGTM: Fee collection migrated to unlocked_amount.All fee collection and refund logic consistently uses
unlocked_amount()throughout the refactored fee finalization flow.
1528-1534: LGTM: Stealth transfer validation uses unlocked_amount.The revealed funds validation correctly compares
bucket.unlocked_amount()against the statement's revealed input amount.
400-405: Review comment is based on a misunderstanding of the validation logic.The condition
!b.is_empty()explicitly targets non-empty buckets, not empty ones:
- Empty buckets (where
is_empty()returnstrue) satisfy the condition and pass validation- Non-empty buckets (where
is_empty()returnsfalse) fail the condition and trigger theDanglingBucketserrorThe validation correctly enforces that all buckets must be empty at transaction finalization. Empty buckets are not flagged as dangling; only buckets with remaining funds are.
Likely an incorrect or invalid review comment.
crates/template_lib_types/src/amount/amount.rs (5)
152-157: LGTM: Signature change improves efficiency.Changing
checked_multo takeother: Self(owned) instead of&Selfeliminates a copy operation sinceSelfisCopy.
279-291: LGTM: Square root implementation handles edge cases.The feature-gated
checked_sqrtcorrectly returnsNonefor negative inputs and handles zero as a special case.
311-319: LGTM: Power operations with proper overflow handling.Both
powandchecked_poware well-implemented, delegating to the underlyingI192operations. Test coverage at lines 520-537 addresses the previous review concern.
408-409: LGTM: Extended operator support via macros.Adding
remandnegto thenewtype_ops!macro provides complete arithmetic operator coverage forAmount.
512-548: LGTM: Comprehensive test coverage added.Tests now cover:
- Basic arithmetic including new
checked_pow(lines 520-521)- Overflow detection for
checked_pow(lines 535-536)- Square root operations including negative input handling (lines 539-548)
This addresses the previous review comment requesting test coverage for power operations.
applications/tari_validator_node/src/genesis_state.rs (1)
126-133: LGTM: Vault initialization refactored cleanly.The vault is now created before the component header and properly referenced in the component state. This removes the need for ResourceManager in genesis state initialization.
crates/engine_types/src/vault.rs (1)
105-111: LGTM: Clear separation of locked vs unlocked balances.The
balance()method now correctly returns only unlocked amounts, whilelocked_balance()(line 109) provides separate access to locked amounts. This is a logical and consistent API design.crates/template_builtin/build.rs (1)
13-18: LGTM: New liquidity pool template added to build.The
liquidity_pooltemplate is now included in the build process alongside the existing templates.crates/template_test_tooling/src/lib.rs (1)
14-14: LGTM: Test utility renamed for clarity.The rename from
test_faucet_componenttoxtr_faucet_componentprovides more specific naming. Note this is a breaking change for any code using the test tooling.crates/engine/tests/composability.rs (2)
227-247: LGTM: Empty account sufficient for composability test.The test focuses on invalid method calls and error handling, not account balances. Using
create_empty_account()simplifies the test setup without affecting the test's validity.
316-352: LGTM: Empty account sufficient for recursion limit test.The test validates recursion depth limits, which doesn't require funded accounts. The change simplifies test setup while preserving test coverage.
crates/template_lib/Cargo.toml (1)
30-30: LGTM: New feature flag for extended arithmetic.The
extra-arithfeature enables extended arithmetic capabilities fromtari_template_lib_types, supporting the new liquidity pool implementation and other advanced use cases.crates/engine/src/runtime/fee_state.rs (1)
26-62: LGTM: Fee validation now uses unlocked amounts.The change to
unlocked_amount()is consistent with the broader API shift and makes semantic sense—fee payments should use unlocked resources. Error messages are correctly updated to reference the unlocked amounts.crates/engine_types/src/proof.rs (1)
84-86: Semantic change verified and isolated to one caller.The search found only one caller: the runtime's
ProofAction::GetAmounthandler atcrates/engine/src/runtime/impl.rs:2179. The method now returns unlocked amounts exclusively (viaLockedResource::amount()→self.locked.unlocked_amount()). The change flows throughProof::amount()→LockedResource::amount()and is returned directly to external callers via the GetAmount action.This is a breaking change if external callers previously relied on total amounts (locked + unlocked). No other callers exist in the codebase.
crates/template_builtin/src/lib.rs (2)
39-41: LGTM: New liquidity pool template address constant.The constant follows the established naming convention and pattern used by other builtin template addresses.
65-69: Appropriate deferral of template inclusion.The commented-out liquidity pool template entry aligns with the PR's partial implementation approach. The path and structure are correct for when the template is ready to be enabled.
crates/template_builtin/templates/liquidity_pool/Cargo.toml (1)
1-19: LGTM: Well-configured template crate.The Cargo.toml follows the established pattern for template crates with appropriate features enabled (macro, alloc, extra-arith) and an optimized release profile for WASM binary size.
crates/engine/tests/test.rs (1)
37-40: LGTM: Import additions for XTR constant and Amount type.These imports support the updated test interactions with resources and amounts. The changes are minimal and appropriate.
crates/template_builtin/templates/faucet/src/lib.rs (1)
43-45: LGTM: Cleaner resource manager access.The change to obtain the
ResourceManagerat call time viaself.vault.to_resource_manager()rather than storing it as a field reduces redundant state. This aligns with the broader API surface changes in the PR.crates/template_lib_types/Cargo.toml (1)
12-13: LGTM: Well-structured optional arithmetic feature.The
extra-arithfeature is properly gated with optionalnum-integerdependency. The?/syntax in thestdfeature correctly handles conditional inclusion ofnum-integer/std. The configuration maintains no_std compatibility by default.Also applies to: 23-23
crates/template_test_tooling/src/read_only_state_store.rs (2)
15-18: LGTM: Import addition for new method.The
TemplateAddressimport is necessary for the newget_components_by_template_addressmethod.
33-48: LGTM: Useful helper method for finding components by template.The implementation correctly filters all components matching the provided template address. The approach is straightforward with proper error handling.
crates/engine/tests/fees.rs (3)
8-8: LGTM: Updated to use new faucet helper.The import change from
test_faucet_componenttoxtr_faucet_componentaligns with the broader test tooling updates in the PR.
74-80: LGTM: Updated faucet interaction pattern.The change to use
xtr_faucet_component()with explicittakemethod calls is more explicit and aligns with the new faucet API.
94-94: LGTM: Consistent faucet usage updates.The balance assertions and faucet calls are updated consistently throughout the test file to use the new
xtr_faucet_component()helper with explicit amounts.Also applies to: 119-119
crates/engine_types/src/bucket.rs (1)
51-62: LGTM: More explicit bucket balance API.The replacement of
amount()with three specific methods (is_empty(),unlocked_amount(),balance()) provides clearer semantics. The distinction between unlocked and total balance is important for locked resource scenarios. The doc comment onbalance()is helpful.crates/template_test_tooling/src/template_test.rs (4)
30-30: LGTM!The new imports support the dynamic template loading and the
create_test_faucet_componenthelper method.Also applies to: 41-41, 44-44
71-73: Good refactoring.The function rename aligns with the XTR faucet refactoring, and extracting the initial balance as a constant improves code maintainability.
Also applies to: 95-96
126-128: LGTM!Dynamic template loading via
all_builtin_templates()is more maintainable than hardcoded static addresses.
440-466: LGTM!The refactored methods correctly use the XTR faucet component's
takemethod, and the#[track_caller]annotations improve debugging experience.Also applies to: 468-499
crates/template_test_tooling/src/builtin_component_state.rs (1)
14-14: LGTM!The import changes correctly support the XTR faucet refactoring and remove unused imports.
Also applies to: 22-22, 26-26, 29-29, 32-32
ab89896 to
4732060
Compare
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 (3)
crates/engine/src/runtime/impl.rs (2)
867-889: Fix action names in error messages and asserts for GetResourceInfoError/label strings still refer to GetResourceType, which is confusing.
- ResourceAction::GetResourceInfo => { + ResourceAction::GetResourceInfo => { let resource_address = resource_ref .as_resource_address() .ok_or_else(|| RuntimeError::InvalidArgument { argument: "resource_ref", - reason: "GetResourceType resource action requires a resource address".to_string(), + reason: "GetResourceInfo resource action requires a resource address".to_string(), })?; - args.assert_no_args("ResourceAction::GetResourceType")?; + args.assert_no_args("ResourceAction::GetResourceInfo")?;Also fix the similar message in GetTotalSupply:
- reason: "GetResourceType resource action requires a resource address".to_string(), + reason: "GetTotalSupply resource action requires a resource address".to_string(),
1701-1712: Typo in argument label (“TakeFeesArg”)This path validates PayFee; the argument label should be consistent.
- if container.unlocked_amount().is_zero() { - return Err(RuntimeError::InvalidArgument { - argument: "TakeFeesArg", + if container.unlocked_amount().is_zero() { + return Err(RuntimeError::InvalidArgument { + argument: "PayFeeArg", reason: "Fee payment has zero value".to_string(), }); }crates/engine_types/src/resource_container.rs (1)
610-626: Stealth lock_all returns wrong variant (type mismatch)The Stealth branch of
lock_all()at line 626 incorrectly returnsSelf::public_fungible(...), creating a Fungible variant instead of Stealth. This violates the type invariant enforced byunlock()at line 631, which checksif self.resource_type() != container.resource_type()and will fail for stealth resources. All other branches (Fungible, NonFungible, Confidential) correctly preserve their variant type.Self::Stealth { revealed_amount, locked_amount, .. } => { if revealed_amount.is_zero() { return Err(ResourceError::InsufficientBalance { details: "lock_all: resource container contained no funds".to_string(), }); } // Sets to zero and returns the amount let newly_locked_amount = mem::take(revealed_amount); *locked_amount += newly_locked_amount; - Ok(Self::public_fungible(resource_address, newly_locked_amount)) + Ok(Self::stealth(resource_address, newly_locked_amount)) },
♻️ Duplicate comments (3)
crates/engine_types/src/resource_container.rs (1)
868-872: is_empty implementation looks correct nowUnlocked + locked + commitments all considered; addresses the prior locked-only buckets case.
crates/engine/src/runtime/impl.rs (1)
2163-2181: DropEmpty removes the bucket before checking emptiness (funds loss on error path)
take_bucketremoves the bucket; if it’s non‑empty you error after removal, losing contents.Apply the safe pattern: inspect in place, then remove.
BucketAction::DropEmpty => { let bucket_id = bucket_ref.bucket_id().ok_or_else(|| RuntimeError::InvalidArgument { argument: "bucket_ref", reason: "DropEmpty bucket action requires a bucket id".to_string(), })?; args.assert_no_args("Bucket::DropEmpty")?; - self.tracker.write_with(|state| { - let bucket = state.take_bucket(bucket_id)?; - if !bucket.is_empty() { - return Err(RuntimeError::InvalidArgument { - argument: "bucket_ref", - reason: "Cannot drop a non-empty bucket".to_string(), - }); - } - // Drop - Ok(InvokeResult::unit()) - }) + self.tracker.write_with(|state| { + { + let bucket = state.get_bucket(bucket_id)?; + if !bucket.is_empty() { + return Err(RuntimeError::InvalidArgument { + argument: "bucket_ref", + reason: "Cannot drop a non-empty bucket".to_string(), + }); + } + } + let _ = state.take_bucket(bucket_id)?; + Ok(InvokeResult::unit()) + }) },Please add a test that calling DropEmpty on a non‑empty bucket leaves it intact.
crates/template_builtin/templates/liquidity_pool/src/lib.rs (1)
97-219: contribute drops residual buckets — return minted LP plus leftover bucketscontribute() partially consumes the input buckets via .take(...) but does not return or explicitly handle the remaining contents; the leftover buckets are dropped on scope exit which, per the Bucket API, will cause a dangling‑bucket panic or asset loss. Change the API to return the minted LP bucket plus the two residual buckets so callers can reclaim leftovers and update call sites/events.
- pub fn contribute(&mut self, mut bucket_a: Bucket, mut bucket_b: Bucket) -> Bucket { + pub fn contribute( + &mut self, + mut bucket_a: Bucket, + mut bucket_b: Bucket, + ) -> (Bucket, Bucket, Bucket) { … let contributed_a = bucket_a.take(a_contribution); let contributed_b = bucket_b.take(b_contribution); … - mint_lp_tokens + (mint_lp_tokens, bucket_a, bucket_b) }Add unit/integration tests that call contribute() with off‑ratio inputs to verify leftovers are returned and no dangling buckets occur.
🧹 Nitpick comments (4)
crates/template_lib/src/models/metadata.rs (1)
53-56: Consider making the string conversion explicit.The method relies on implicit coercion from
&mut String(returned byor_insert_with) to&str. While this works correctly, explicitly calling.as_str()would improve readability.Apply this diff to make the conversion explicit:
pub fn get_or_insert<K: Into<String>, V: Into<String>>(&mut self, key: K, default: V) -> &str { let key = key.into(); - self.0.entry(key).or_insert_with(|| default.into()) + self.0.entry(key).or_insert_with(|| default.into()).as_str() }crates/engine_types/src/proof.rs (1)
84-86: Consider: Naming inconsistency in LockedResource::amount().The method
amount()onLockedResourcenow returnsunlocked_amount(), which is semantically confusing—one would expectLockedResource.amount()to return the locked amount, not the unlocked amount. While this aligns with the broader refactor, consider whether this method should be renamed tounlocked_amount()for clarity, or whether it should actually return the locked amount instead.crates/engine/src/transaction/processor.rs (1)
333-346: Post-take cleanup: prefer clearer naming and explicit intentThe emptiness probe is correct (Everything covers locked + commitments). Consider a small refactor to clarify semantics and reduce confusion:
- let prev_bucket_val = runtime + // Remaining total after take (unlocked + locked + commitments) + let remaining_total = runtime .interface() .bucket_invoke( bucket_ref, BucketAction::GetAmount, invoke_args![BucketGetAmountArg::Everything].into(), )? .decode::<Amount>()?; - if prev_bucket_val.is_zero() { + if remaining_total.is_zero() { // Drop the bucket to prevent a dangling (empty) bucket runtime .interface() .bucket_invoke(bucket_ref, BucketAction::DropEmpty, invoke_args![].into())?; }Please confirm this path is exercised for confidential/stealth buckets (commitments-only) in tests so we don’t regress emptiness detection.
crates/engine/src/runtime/working_state.rs (1)
399-405: DanglingBuckets count misreports when only some buckets are non‑emptyYou error if any non‑empty bucket exists, but report the total bucket count. Report only the non‑empty ones to aid debugging.
- if self.buckets.iter().any(|(_, b)| !b.is_empty()) { - return Err(TransactionCommitError::DanglingBuckets { - count: self.buckets.len(), - } + let non_empty = self.buckets.values().filter(|b| !b.is_empty()).count(); + if non_empty > 0 { + return Err(TransactionCommitError::DanglingBuckets { count: non_empty } .into()); }If the intent is to forbid any leftover buckets (even empty), change the predicate to
!self.buckets.is_empty()and keepcount: self.buckets.len(). Please confirm desired policy.
📜 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 (50)
Cargo.toml(2 hunks)applications/tari_indexer/README.md(1 hunks)applications/tari_validator_node/src/genesis_state.rs(2 hunks)crates/engine/src/runtime/fee_state.rs(1 hunks)crates/engine/src/runtime/impl.rs(13 hunks)crates/engine/src/runtime/working_state.rs(9 hunks)crates/engine/src/transaction/processor.rs(2 hunks)crates/engine/src/wasm/module.rs(2 hunks)crates/engine/src/wasm/process.rs(1 hunks)crates/engine/tests/access_rules.rs(1 hunks)crates/engine/tests/account.rs(3 hunks)crates/engine/tests/composability.rs(2 hunks)crates/engine/tests/events.rs(2 hunks)crates/engine/tests/fees.rs(4 hunks)crates/engine/tests/templates/buggy/src/lib.rs(1 hunks)crates/engine/tests/test.rs(2 hunks)crates/engine_types/src/bucket.rs(1 hunks)crates/engine_types/src/commit_result.rs(7 hunks)crates/engine_types/src/events.rs(1 hunks)crates/engine_types/src/proof.rs(1 hunks)crates/engine_types/src/resource.rs(1 hunks)crates/engine_types/src/resource_container.rs(12 hunks)crates/engine_types/src/vault.rs(1 hunks)crates/template_abi/src/version.rs(1 hunks)crates/template_builtin/build.rs(1 hunks)crates/template_builtin/src/lib.rs(2 hunks)crates/template_builtin/templates/account/Cargo.toml(1 hunks)crates/template_builtin/templates/account/src/lib.rs(1 hunks)crates/template_builtin/templates/faucet/Cargo.toml(1 hunks)crates/template_builtin/templates/faucet/src/lib.rs(1 hunks)crates/template_builtin/templates/liquidity_pool/Cargo.toml(1 hunks)crates/template_builtin/templates/liquidity_pool/src/lib.rs(1 hunks)crates/template_builtin/tests/liquidity_pool.rs(1 hunks)crates/template_lib/Cargo.toml(1 hunks)crates/template_lib/src/args/types.rs(3 hunks)crates/template_lib/src/models/account.rs(1 hunks)crates/template_lib/src/models/bucket.rs(2 hunks)crates/template_lib/src/models/metadata.rs(2 hunks)crates/template_lib/src/resource/manager.rs(2 hunks)crates/template_lib_types/Cargo.toml(2 hunks)crates/template_lib_types/src/amount/amount.rs(6 hunks)crates/template_lib_types/src/resource_type.rs(2 hunks)crates/template_test_tooling/src/builtin_component_state.rs(2 hunks)crates/template_test_tooling/src/lib.rs(1 hunks)crates/template_test_tooling/src/package_builder.rs(2 hunks)crates/template_test_tooling/src/read_only_state_store.rs(2 hunks)crates/template_test_tooling/src/support/assert_error.rs(2 hunks)crates/template_test_tooling/src/template_test.rs(19 hunks)integration_tests/Cargo.toml(2 hunks)lints.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (24)
- crates/template_builtin/templates/account/src/lib.rs
- crates/engine_types/src/vault.rs
- crates/template_lib/src/models/account.rs
- crates/template_lib/src/args/types.rs
- crates/engine/tests/fees.rs
- crates/template_test_tooling/src/read_only_state_store.rs
- crates/template_builtin/templates/faucet/src/lib.rs
- crates/template_lib_types/Cargo.toml
- crates/engine/src/wasm/module.rs
- applications/tari_indexer/README.md
- crates/engine/tests/composability.rs
- crates/template_lib/src/resource/manager.rs
- crates/template_lib/Cargo.toml
- crates/engine/tests/access_rules.rs
- crates/template_builtin/templates/account/Cargo.toml
- crates/template_builtin/tests/liquidity_pool.rs
- crates/engine/src/runtime/fee_state.rs
- applications/tari_validator_node/src/genesis_state.rs
- lints.toml
- crates/engine/tests/events.rs
- crates/template_test_tooling/src/support/assert_error.rs
- crates/engine/tests/test.rs
- crates/template_builtin/src/lib.rs
- crates/template_builtin/templates/faucet/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (15)
crates/template_test_tooling/src/template_test.rs (2)
crates/template_builtin/src/lib.rs (1)
all_builtin_templates(51-72)crates/template_test_tooling/src/builtin_component_state.rs (1)
initialize_builtin_faucet_state(82-115)
crates/engine_types/src/commit_result.rs (1)
crates/engine_types/src/fees.rs (1)
is_paid_in_full(58-60)
crates/engine/tests/account.rs (1)
crates/template_test_tooling/src/template_test.rs (1)
xtr_faucet_component(71-73)
crates/engine/src/runtime/working_state.rs (3)
crates/engine_types/src/resource_container.rs (3)
public_fungible(55-63)resource_address(175-182)unlocked_amount(134-141)crates/engine_types/src/bucket.rs (2)
resource_address(67-69)unlocked_amount(55-57)crates/engine_types/src/proof.rs (4)
resource_address(46-48)resource_address(88-90)amount(42-44)amount(84-86)
crates/template_lib/src/models/bucket.rs (3)
crates/template_lib/src/models/proof.rs (4)
resp(132-132)resp(175-175)resp(195-195)amount(104-112)crates/engine_types/src/bucket.rs (2)
is_empty(51-53)locked_amount(63-65)crates/engine_types/src/resource_container.rs (2)
is_empty(868-872)locked_amount(154-163)
crates/template_lib/src/models/metadata.rs (2)
crates/engine_types/src/indexed_value.rs (2)
metadata(105-107)metadata(313-315)crates/engine_types/src/resource.rs (1)
metadata(203-205)
crates/engine/src/transaction/processor.rs (2)
bindings/src/types/Amount.ts (1)
Amount(12-12)crates/template_lib/src/models/bucket.rs (1)
amount(206-214)
crates/template_test_tooling/src/package_builder.rs (2)
crates/template_builtin/src/lib.rs (1)
get_template_builtin(43-45)crates/engine/src/wasm/module.rs (1)
from_code(61-63)
crates/template_test_tooling/src/lib.rs (1)
crates/template_test_tooling/src/template_test.rs (1)
xtr_faucet_component(71-73)
crates/engine/src/runtime/impl.rs (4)
crates/engine_types/src/resource.rs (2)
divisibility(211-213)resource_type(107-109)crates/template_lib/src/resource/manager.rs (2)
divisibility(129-131)resource_type(119-121)crates/engine_types/src/resource_container.rs (2)
resource_type(184-191)public_fungible(55-63)crates/template_lib/src/models/bucket.rs (2)
resource_type(88-97)amount(206-214)
crates/engine_types/src/resource_container.rs (5)
crates/template_lib/src/resource/builder/mod.rs (1)
public_fungible(54-56)crates/template_lib/src/models/bucket.rs (4)
amount(206-214)resource_address(76-85)locked_amount(219-227)is_empty(185-194)crates/engine_types/src/proof.rs (6)
amount(42-44)amount(84-86)resource_address(46-48)resource_address(88-90)container(58-60)container(100-102)crates/template_lib/src/models/proof.rs (2)
amount(104-112)resource_address(71-80)crates/engine_types/src/bucket.rs (4)
unlocked_amount(55-57)resource_address(67-69)locked_amount(63-65)is_empty(51-53)
crates/template_test_tooling/src/builtin_component_state.rs (4)
crates/template_test_tooling/src/template_test.rs (2)
test_nft_faucet_component(75-77)xtr_faucet_component(71-73)crates/engine_types/src/resource_container.rs (1)
stealth(91-101)crates/template_lib/src/models/vault.rs (1)
for_test(478-480)bindings/src/types/ComponentHeader.ts (1)
ComponentHeader(9-17)
crates/template_builtin/templates/liquidity_pool/src/lib.rs (4)
crates/engine_types/src/resource_container.rs (3)
public_fungible(55-63)resource_address(175-182)resource_type(184-191)crates/engine_types/src/vault.rs (3)
new(49-54)resource_address(121-123)resource_type(125-127)crates/engine/src/runtime/impl.rs (1)
emit_event(436-459)crates/template_lib/src/models/bucket.rs (3)
resource_address(76-85)amount(206-214)resource_type(88-97)
crates/engine_types/src/bucket.rs (2)
crates/engine_types/src/resource_container.rs (2)
is_empty(868-872)unlocked_amount(134-141)crates/template_lib/src/models/bucket.rs (1)
is_empty(185-194)
crates/template_lib_types/src/resource_type.rs (1)
bindings/src/types/ResourceType.ts (1)
ResourceType(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check nightly
- GitHub Check: check stable
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: machete
- GitHub Check: fmt
🔇 Additional comments (48)
crates/engine/tests/templates/buggy/src/lib.rs (1)
46-50: LGTM! Version bump aligns with breaking changes.The embedded ABI template version has been correctly updated to reflect the new template ABI version introduced in this PR. This change is expected given the breaking changes to the template ABI mentioned in the PR objectives.
crates/engine_types/src/commit_result.rs (1)
62-62: LGTM! Excellent improvement to test helper ergonomics.Adding
#[track_caller]to these test assertion helpers will significantly improve the developer experience by showing the actual test location where assertions fail, rather than the internal helper method location. This is a standard Rust pattern for methods that panic and is correctly applied to all relevant methods.Also applies to: 69-69, 81-81, 90-90, 99-99, 110-110, 119-119, 128-128, 135-135
crates/template_builtin/build.rs (1)
13-18: LGTM! Template structure verified.The addition of the liquidity_pool template to the build list is correct. The template directory has the required structure with Cargo.toml and src/lib.rs in place.
crates/engine_types/src/resource.rs (1)
207-209: Clean simplification—change is correct and consistent with metadata API.The
Metadata::get()method signature ispub fn get(&self, key: &str) -> Option<&str>, confirming that it returnsOption<&str>directly. The removal of.map(|s| s.as_str())is appropriate and eliminates redundant mapping. The public API remains stable with the unchanged method signature.crates/engine/src/wasm/process.rs (1)
276-290: LGTM! Version constant migration is consistent.The migration from
MINIMUM_SUPPORTED_TEMPLATE_LIB_VERSIONtoMINIMUM_SUPPORTED_WASM_ABI_VERSIONis applied consistently across all references (lines 279, 282, 284), and the updated log messages accurately reflect the ABI-centric versioning approach.crates/template_abi/src/version.rs (2)
5-5: Version bump looks good.The update to "0.15.0" aligns with the breaking changes and new features introduced in this PR.
6-7: Verification passed: old constant completely removed and new constant properly integrated.The old
MINIMUM_SUPPORTED_TEMPLATE_LIB_VERSIONconstant has been completely removed with no orphaned references remaining in the codebase. The newMINIMUM_SUPPORTED_WASM_ABI_VERSIONconstant is consistently used across all necessary locations inprocess.rs. The breaking change (setting minimum = latest at "0.15.0") is intentional and complete.crates/template_lib/src/models/metadata.rs (3)
49-51: LGTM! Idiomatic API improvement.Changing the return type from
Option<&String>toOption<&str>is more idiomatic and provides better ergonomics for callers. The implementation correctly usesmap(|s| s.as_str())to perform the conversion.
58-60: LGTM! Clean implementation.The
removemethod correctly delegates to the inner BTreeMap and returns the removed value, providing a useful addition to the API.
182-186: LGTM! Tests properly updated.The test assertions correctly reflect the new return type of
Option<&str>for theget()method.crates/template_test_tooling/src/package_builder.rs (3)
66-69: Breaking change toadd_templatereturn type acknowledged.The method now returns
TemplateAddressinstead of&mut Self, breaking the fluent API. This is documented in the PR objectives and allows callers to directly obtain the template address. Users who need method chaining can useadd_template_with_envsor calladd_template_optsand handle the address separately.
106-109: Good addition ofadd_template_from_codehelper.This new public method provides a clean abstraction for loading templates from raw WASM code. The signature aligns well with
WasmModule::from_code, and the use ofunwrap()is consistent with existing error handling in this test tooling crate.
101-104: No issues found.TemplateAddressis a type alias forHash, which explicitly derivesCopy. TakingTemplateAddressby value inadd_builtin_template()is safe and allows the caller to continue using the value after the call.crates/template_test_tooling/src/lib.rs (1)
14-14: LGTM! Clean rename to align with XTR faucet naming convention.The export rename from
test_faucet_componenttoxtr_faucet_componentis consistent with the broader refactoring across the PR to standardize faucet naming.crates/template_test_tooling/src/template_test.rs (7)
30-30: LGTM! Imports support the new dynamic template loading and vault extraction features.The added imports for
IndexedWellKnownTypes,all_builtin_templates, andResourceAddressenable the dynamic builtin template loading and the new faucet component creation helper.Also applies to: 41-41, 44-44
71-73: LGTM! Function rename aligns with XTR faucet naming.The rename from
test_faucet_componenttoxtr_faucet_componentis consistent with the broader refactoring to standardize XTR faucet references.
95-96: LGTM! Extracting the initial balance as a constant improves maintainability.The new
FUNDED_ACCOUNT_INITIAL_BALANCEconstant makes the test configuration explicit and easier to adjust. Good practice to avoid magic numbers.
126-128: LGTM! Dynamic template loading improves maintainability.Using
all_builtin_templates()instead of hardcoded addresses makes the codebase more maintainable and flexible for adding new builtin templates.
320-320: LGTM! The#[track_caller]attributes improve debugging experience.Adding
#[track_caller]to test helper methods that may panic provides better error context by preserving the actual caller location. This is especially valuable in test tooling.Also applies to: 345-345, 375-375, 426-426, 444-444, 472-472, 506-506, 553-553, 560-560, 575-575, 641-641, 699-699, 708-708, 733-733
451-453: LGTM! Consistent usage of renamed function and extracted constant.The updates to use
xtr_faucet_component()andFUNDED_ACCOUNT_INITIAL_BALANCEalign with the refactoring and improve code clarity.Also applies to: 487-487
507-545: LGTM! The new helper consolidates faucet creation logic.The
create_test_faucet_componentmethod consolidates previously duplicated faucet setup code. The assumptions about component structure (having at least one vault) are reasonable for test faucets, and the.expect()message provides clear debugging context if the assumption is violated.crates/engine/tests/account.rs (2)
16-16: LGTM! Import updated to match the renamed function.The import change from
test_faucet_componenttoxtr_faucet_componentis consistent with the refactoring.
25-25: LGTM! Test setup simplified using the new faucet helper.Line 25 now uses the
create_test_faucet_componenthelper, which simplifies the test setup and eliminates manual faucet component creation logic. Line 232 consistently uses the renamedxtr_faucet_component()function.Also applies to: 232-232
crates/template_test_tooling/src/builtin_component_state.rs (3)
14-14: LGTM! Import updates align with XTR faucet refactoring.The import changes bring in the necessary XTR faucet constants (
XTR_FAUCET_TEMPLATE_ADDRESS,XTR_FAUCET_VAULT_ADDRESS) and updated function references, supporting the transition from generic test faucet to XTR-specific faucet implementation.Also applies to: 22-22, 26-26, 29-29, 32-32
86-86: LGTM! UsingAmount::MAXensures the test faucet never runs out of tokens.Changing the initial supply from
1_000_000toAmount::MAXis appropriate for test infrastructure, ensuring that tests have sufficient tokens regardless of their consumption patterns.
93-93: LGTM! Consistent application of XTR faucet constants and naming.The changes consistently update the builtin faucet state initialization to use XTR-specific constants (
XTR_FAUCET_VAULT_ADDRESS,XTR_FAUCET_TEMPLATE_ADDRESS) and naming ("XtrFaucet"module name,xtr_faucet_component()function). Using predefined constants instead of derived IDs improves maintainability.Also applies to: 98-98, 103-103, 105-106
crates/template_lib_types/src/amount/amount.rs (7)
152-157: LGTM: Signature change improves consistency.Changing the parameter from
other: &Selftoother: Selfalignschecked_mulwith the other checked arithmetic methods (checked_add,checked_sub,checked_div), which all takeSelfby value. This improves API consistency.
311-319: LGTM: Previous test coverage concern has been addressed.The
powandchecked_powimplementations are straightforward. Test coverage for these methods has been added (lines 520-521, 535-536), addressing the concern raised in the previous review.
408-409: LGTM: Macro-based operator implementations.The updated
newtype_ops!invocations now includeremandneg, generating implementations for remainder and negation operations. This is cleaner than manual implementations and ensures consistency across all operator traits.
512-537: LGTM: Comprehensive test coverage.The test coverage is thorough, including:
- Basic arithmetic operations with the updated
checked_mulsignature- New
checked_powfunctionality (lines 520-521)- Overflow detection for all checked operations (lines 523-536)
This addresses the previous review concern about missing test coverage for power operations.
539-548: LGTM: Feature-gated tests forchecked_sqrt.The tests for the
extra-arithfeature are correctly gated and cover both the success case (integer square root) and the error case (negative input returnsNone).
7-7: Neg removal from imports is correct and verified.The
newtype_ops!macro includesnegin its operation list (lines 408-409) and correctly generates theNegimplementation for bothSelfand&Selfforms. No manualNegimplementation remains, and negation is working correctly across all test cases (checked_sub returning negative values, unary minus operator usage, etc.).
279-291: No issues found - dependency configuration is correct.The
num-integerdependency is properly declared inCargo.toml(line 23) withoptional = true, and theextra-arithfeature correctly enables it using the"dep:num-integer"syntax. Thechecked_sqrt()implementation properly gates the feature-dependent code, and tests confirm it works as intended (sqrt(27) returns 5; negative values return None).crates/engine_types/src/events.rs (1)
115-117: LGTM! Simplified return value handling.The removal of the
.map(|s| s.as_str())mapping suggests thatMetadata::getnow returnsOption<&str>directly, making this cleaner and more direct.integration_tests/Cargo.toml (1)
12-13: LGTM! Dependency sourcing aligned with workspace changes.The switch from workspace-based to git-based dependencies for Minotari components and the addition of
tari_commsdependencies align with the workspace-level dependency cleanup and ensure integration tests use the specified v5.1.0-rc.1 tag.Also applies to: 26-26, 49-50
Cargo.toml (2)
3-3: Version bump reflects breaking changes.The bump from 0.15.1 to 0.16.0 correctly signals the breaking changes documented in the PR objectives (data directory deletion required, template ABI changes).
213-213: New dependency for arithmetic operations.The
num-integerdependency supports theextra-arithfeature used in the liquidity pool template (seecrates/template_builtin/templates/liquidity_pool/Cargo.tomlline 8).crates/template_lib/src/models/bucket.rs (4)
172-182: LGTM! Proper cleanup mechanism for empty buckets.The
drop_empty()method provides an explicit way to dispose of empty buckets, preventing dangling bucket errors. The panic on non-empty buckets is appropriate defensive programming.
196-214: Documentation and implementation clarified.The updated documentation clearly describes the behavior for different resource types and the distinction between unlocked amounts and locked/confidential funds. The use of
BucketGetAmountArg::AmountOnlymakes the intent explicit.
216-227: LGTM! New locked_amount accessor.Provides access to locked amounts separately from unlocked amounts, supporting the broader API refactor to distinguish between locked and unlocked balances.
184-194: Review comment is incorrect.The implementation correctly detects confidential commitments. The engine-side
BucketGetAmountArg::Everythinghandler atcrates/engine/src/runtime/impl.rs:1959-1974explicitly addsbucket.number_of_confidential_commitments()to the total amount returned. Therefore, a bucket with zero revealed balance but non-zero confidential commitments will return a non-zero amount, andis_empty()will correctly returnfalse.The concern that
drop_empty()could panic due tois_empty()incorrectly reporting an empty bucket is unfounded—the current implementation prevents this scenario.Note: The engine implementation contains a bug where confidential commitment count is added twice (lines 1964 and 1967), but this does not affect the correctness of the
is_empty()logic since any non-zero commitment count still results in a non-zero total.Likely an incorrect or invalid review comment.
crates/template_lib_types/src/resource_type.rs (2)
64-96: LGTM! Clean feature-gated parsing with proper exports.The parsing logic is now cleanly isolated behind the
stdfeature gate, and the past review concern aboutParseResourceTypeErrorvisibility has been properly addressed with the re-export on line 96.
98-102: New ResourceInfo struct exposes divisibility.The addition of
ResourceInfoprovides a structured way to return both resource type and divisibility information together, addressing the ABI gap mentioned in the PR objectives.crates/template_builtin/templates/liquidity_pool/Cargo.toml (1)
1-19: LGTM! Well-configured liquidity pool template.The template configuration properly enables the
extra-arithfeature for arithmetic operations (supporting the liquidity pool calculations) and includes appropriate release optimizations for WASM compilation.crates/engine_types/src/bucket.rs (1)
51-57: LGTM! API refactor aligns with ResourceContainer changes.The replacement of
amount()withis_empty()andunlocked_amount()provides clearer semantics and properly delegates to the engine-sideResourceContainerimplementation, which correctly checks all resource states including confidential commitments.crates/engine/src/runtime/working_state.rs (1)
1328-1371: Fee collection/refund migration to unlocked_amount is consistentUsing unlocked_amount for fee accounting and refunds aligns with new semantics.
crates/engine/src/runtime/impl.rs (1)
1383-1393: Vault::Create initializes fungible vaults via public_fungible(…, 0) — LGTMThis aligns with the new constructor and avoids negative/locked states.
crates/template_builtin/templates/liquidity_pool/src/lib.rs (1)
331-344: Fungibility check is correct and matches engine variantsAccepting Fungible, Confidential, and Stealth as “fungible‑like” is consistent with ResourceType semantics.
2 participants
Description
feat: partial implementation of builtin liquidity pool
feat!: Adds ability to get divisibility from ResourceManager
feat!: adds
drop_emptyand is empty calls to bucketMotivation and Context
Liquidity pools are generally useful (e.g. for swap implementations, fee paying fees using other resources, etc)
The ABI missed a way to get the resource divisibility
How Has This Been Tested?
Partialy in existing unit tests, more tests to follow once a more complete implementation is done
What process can a PR reviewer use to test or verify this change?
Breaking Changes
BREAKING CHANGE: template ABI changes will require all templates using resource_type calls to be recompiled
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Version