feat: allow withdrawing assets from pool to owner#12
Conversation
# Conflicts: # contracts/intents-vault/res/intents_vault.wasm # contracts/intents-vault/src/lib.rs # contracts/mock-intents/res/mock_intents.wasm # contracts/mock-intents/src/lib.rs # contracts/solver-registry/res/solver_registry.wasm # contracts/solver-registry/src/admin.rs # contracts/solver-registry/src/events.rs # contracts/solver-registry/src/ext/intents_vault.rs
WalkthroughAdds multi-token accounting and FT withdrawal paths, a TokenId type, PromiseOrValue ft_withdraw signatures and callbacks, ONE_YOCTO constant, event payloads changed to owned values, removal of the upgrade module, new integration tests, and CI/Makefile/Cargo metadata updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Owner
participant SR as Solver Registry
participant IV as Intents Vault
participant IC as Intents Contract
Owner->>SR: withdraw_from_pool(pool_id, receiver_id, token_id, amount) [+1 yocto]
SR->>SR: validate pool, token, permissions
SR->>IV: ft_withdraw(intents_id, token, receiver, amount, memo, msg) [+1 yocto]
IV->>IV: assert 1 yocto && caller == parent
IV->>IC: ft_withdraw(token, receiver, amount, memo, msg)
IC-->>IV: PromiseOrValue<U128> (used_amount)
IV-->>SR: PromiseOrValue<U128> (used_amount)
SR->>SR: on_withdraw_from_pool callback(result)
alt Ok
SR->>Owner: return used_amount
SR-->>SR: emit AssetWithdrawn
else Err
SR->>Owner: panic / error
end
sequenceDiagram
autonumber
actor Funder
participant SR as Solver Registry
participant IV as Intents Vault
participant IC as Intents Contract
Funder->>SR: deposit_into_pool(pool_id, token_id, amount) [+1 yocto]
SR->>IV: ft_on_transfer / deposit flow
IV->>IV: internal_deposit_mt_balance(TokenId, receiver, amount)
IV->>IC: external FT ops (transfer/resolve)
IC-->>IV: PromiseOrValue<U128> (used_amount)
IV-->>SR: used_amount
SR->>SR: on_deposit_into_pool(pool_id, token_id, amount, result)
alt Ok and used_amount>0
SR-->>SR: emit AssetDeposited
SR->>Funder: refund amount - used_amount
else Ok and used_amount==0
SR->>Funder: refund full amount
else Err
SR->>Funder: panic / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/solver-registry/tests/common/utils.rs (1)
285-334: Critical: ensure owner is FT-registered before withdrawalsWithdraw sends FT to receiver_id; NEP‑141 requires receiver storage registration. Owner isn’t registered in setup, so withdrawals to owner will fail when FT transfers execute.
Apply in setup_test_environment:
- // Register accounts for NEP-141 tokens - register_accounts_for_tokens(&wnear, &usdc, &[&alice, &bob]).await?; + // Register accounts for NEP-141 tokens (owner must be able to receive withdrawals) + register_accounts_for_tokens(&wnear, &usdc, &[&owner, &alice, &bob]).await?;
🧹 Nitpick comments (6)
contracts/mock-intents/src/token.rs (2)
12-19: Minor: avoid magic string for default standardUse a constant to prevent typos and centralize the default.
+const NEP141: &str = "nep141"; @@ impl From<AccountId> for TokenId { fn from(account_id: AccountId) -> Self { Self { account_id, - standard: "nep141".to_string(), + standard: NEP141.to_string(), } } }
5-10: Future-proof JSON shape if exposed externallyIf TokenId is ever part of the public JSON API, consider serializing as a single "standard:account_id" string (custom Serialize/Deserialize) to match parsing and logs. Current derive will emit an object. Optional now.
Also applies to: 21-31, 33-43
contracts/solver-registry/tests/test_withdraw_assets.rs (1)
81-93: Optional: assert receiver balance increasesFor stronger E2E validation, also assert owner’s FT balances increased by the withdrawal amounts (after registering owner storage as suggested).
Also applies to: 119-131
contracts/solver-registry/src/pool.rs (2)
17-17: Callback gas likely too low if you add any follow-up actions5 Tgas is tight even for JSON event emission; if you later forward refunds or update storage, you will run out. Recommend 15–20 Tgas for safety.
-const GAS_DEPOSIT_INTO_POOL_CALLBACK: Gas = Gas::from_tgas(5); +const GAS_DEPOSIT_INTO_POOL_CALLBACK: Gas = Gas::from_tgas(20);Based on learnings.
27-47: Minor docs: ‘NEAR’ amounts wording is misleading for multi-token poolsThese vectors track per-token balances, not NEAR. Please update doc comments here and in PoolInfo to avoid confusion.
- /// How much NEAR this contract has. + /// Per-token balances tracked for the pool (indexed by `token_ids`).contracts/intents-vault/src/lib.rs (1)
96-103: Parent-account check can panic on top-level accounts; add a clearer error or fallbacksplit_once('.') will panic for single-segment accounts. If this contract might be deployed outside a subaccount, prefer an explicit check and error message.
- let (_, parent_account_id) = contract_id.split_once('.').expect("Invalid contract ID"); + let Some((_, parent_account_id)) = contract_id.split_once('.') else { + env::panic_str("Contract must be deployed as a subaccount for parent-only access control"); + };Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
contracts/intents-vault/res/intents_vault.wasmis excluded by!**/*.wasmcontracts/mock-ft/res/mock_ft.wasmis excluded by!**/*.wasmcontracts/mock-intents/res/mock_intents.wasmis excluded by!**/*.wasmcontracts/solver-registry/res/solver_registry.wasmis excluded by!**/*.wasm
📒 Files selected for processing (12)
contracts/intents-vault/src/lib.rs(2 hunks)contracts/mock-intents/src/lib.rs(4 hunks)contracts/mock-intents/src/token.rs(1 hunks)contracts/mock-intents/src/token_receiver.rs(2 hunks)contracts/solver-registry/src/admin.rs(3 hunks)contracts/solver-registry/src/events.rs(2 hunks)contracts/solver-registry/src/ext/intents_vault.rs(1 hunks)contracts/solver-registry/src/lib.rs(6 hunks)contracts/solver-registry/src/pool.rs(4 hunks)contracts/solver-registry/src/upgrade.rs(0 hunks)contracts/solver-registry/tests/common/utils.rs(2 hunks)contracts/solver-registry/tests/test_withdraw_assets.rs(1 hunks)
💤 Files with no reviewable changes (1)
- contracts/solver-registry/src/upgrade.rs
🧰 Additional context used
🧬 Code graph analysis (7)
contracts/solver-registry/tests/common/utils.rs (1)
contracts/solver-registry/src/admin.rs (1)
withdraw_from_pool(61-93)
contracts/solver-registry/tests/test_withdraw_assets.rs (3)
contracts/solver-registry/tests/common/utils.rs (11)
setup_test_environment(285-334)create_liquidity_pool(337-359)approve_compose_hash(362-380)register_worker_alice(411-426)create_account(22-34)storage_deposit(97-112)ft_transfer(114-132)deposit_into_pool(187-212)get_pool_account_id(533-537)get_mock_intents_balances(559-576)withdraw_from_pool(579-601)contracts/solver-registry/src/pool.rs (3)
create_liquidity_pool(90-115)deposit_into_pool(181-210)get_pool_account_id(175-179)contracts/solver-registry/src/admin.rs (2)
approve_compose_hash(17-27)withdraw_from_pool(61-93)
contracts/intents-vault/src/lib.rs (2)
contracts/mock-intents/src/lib.rs (3)
add_public_key(53-60)remove_public_key(63-69)ft_withdraw(72-91)contracts/solver-registry/src/ext/intents_vault.rs (3)
add_public_key(6-6)remove_public_key(7-7)ft_withdraw(8-15)
contracts/solver-registry/src/admin.rs (2)
contracts/solver-registry/src/pool.rs (2)
new(59-77)get_pool_account_id(175-179)contracts/solver-registry/tests/common/utils.rs (2)
withdraw_from_pool(579-601)get_pool_account_id(533-537)
contracts/mock-intents/src/lib.rs (4)
contracts/solver-registry/tests/common/utils.rs (1)
ft_transfer(114-132)contracts/mock-ft/src/lib.rs (2)
ft_transfer(82-84)new(59-76)contracts/intents-vault/src/lib.rs (2)
ft_withdraw(32-38)ft_withdraw(77-93)contracts/solver-registry/src/ext/intents_vault.rs (1)
ft_withdraw(8-15)
contracts/solver-registry/src/pool.rs (1)
contracts/solver-registry/tests/common/utils.rs (1)
get_pool_account_id(533-537)
contracts/solver-registry/src/ext/intents_vault.rs (2)
contracts/mock-intents/src/lib.rs (1)
ft_withdraw(72-91)contracts/intents-vault/src/lib.rs (2)
ft_withdraw(32-38)ft_withdraw(77-93)
🪛 GitHub Actions: Test
contracts/solver-registry/tests/common/utils.rs
[error] 1-1: Cargo build failed for solver-registry: could not compile due to previous error (include_bytes missing wasm).
contracts/solver-registry/tests/test_withdraw_assets.rs
[error] 1-1: Cargo build failed for solver-registry: could not compile due to previous error (include_bytes missing wasm).
contracts/solver-registry/src/admin.rs
[error] 1-1: Cargo build failed for solver-registry: could not compile due to previous error (include_bytes missing wasm).
contracts/solver-registry/src/pool.rs
[error] 1-1: Cargo build failed for solver-registry: could not compile due to previous error (include_bytes missing wasm).
contracts/solver-registry/src/events.rs
[error] 1-1: Cargo build failed for solver-registry: could not compile due to previous error (include_bytes missing wasm).
contracts/solver-registry/src/ext/intents_vault.rs
[error] 1-1: Cargo build failed for solver-registry: could not compile due to previous error (include_bytes missing wasm).
contracts/solver-registry/src/lib.rs
[error] 1-1: Cargo build failed for solver-registry: could not compile due to previous error (include_bytes missing wasm).
🪛 GitHub Actions: Build
contracts/solver-registry/src/pool.rs
[error] 108-108: couldn't read contracts/solver-registry/src/../../intents-vault/res/intents_vault.wasm: No such file or directory (os error 2) while expanding include_bytes! macro at this location.
🔇 Additional comments (10)
contracts/mock-intents/src/token_receiver.rs (1)
22-25: LGTM: correct TokenId usage and MT depositConverting predecessor to TokenId and depositing before logging is sound. Return U128(0) complies with NEP-141 expectations.
Please confirm internal_deposit_mt_balance initializes (account_id, token_id) entries to 0 on first use to avoid panics on empty state.
contracts/solver-registry/tests/common/utils.rs (4)
368-369: Verify 1 yocto on approve_compose_hashContract snippet shows approve_compose_hash without assert_one_yocto; if not required, drop the deposit to save gas. If required elsewhere, keep.
539-556: Nice: single‑token MT balance helperHelper is clean and consistent with TokenId string format.
558-576: Batch MT balances helper looks goodEfficient conversion to "nep141:{id}" and mapping to u128.
578-601: Admin withdraw helper matches contract API1 yocto deposit and gas are appropriate.
contracts/solver-registry/src/ext/intents_vault.rs (1)
1-16: LGTM: ext interface matches withdraw flowReturn type and params align with intents-vault and admin usage; ONE_YOCTO is attached at the call site.
contracts/solver-registry/src/pool.rs (2)
181-210: Good: ONE_YOCTO on ft_transfer_call and input validationThe minimal deposit and basic guards look correct for NEP‑141 transfer calls and pool membership checks. No action needed.
108-109: Stale CI blocker – noinclude_bytes!call orintents-vault/resdirectory existsLikely an incorrect or invalid review comment.
contracts/intents-vault/src/lib.rs (1)
76-93: ft_withdraw forwarding looks correct with access control and 1 yⓃEnforces 1 yⓃ and parent-only, forwards to external contract, and preserves the return value. LGTM.
contracts/solver-registry/src/events.rs (1)
50-62: Event additions/shape changes are consistent; amount should reflect used valueNew variants and by-value fields look good. Ensure deposit emits the used amount (not requested) to keep event logs semantically correct — see pool.rs callback note.
Run tests that assert AssetDeposited.amount equals the actually used amount from ft_transfer_call.
| fn ft_transfer( | ||
| receiver_id: AccountId, | ||
| amount: U128, | ||
| memo: Option<String>, | ||
| ) -> PromiseOrValue<U128>; | ||
| } |
There was a problem hiding this comment.
Correct ext_ft::ft_transfer return type to match NEP-141
Line 18 defines ft_transfer as returning PromiseOrValue<U128>, but the NEP-141 standard (and our mock-ft implementation) return (). Expecting a serialized PromiseOrValue<U128> causes the promise to deserialize garbage and fail every withdrawal. This makes the new owner withdrawal path unusable. Please update the trait to return () so the promise resolves successfully.
trait FungibleTokenContract {
fn ft_transfer(
receiver_id: AccountId,
amount: U128,
memo: Option<String>,
- ) -> PromiseOrValue<U128>;
+ );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn ft_transfer( | |
| receiver_id: AccountId, | |
| amount: U128, | |
| memo: Option<String>, | |
| ) -> PromiseOrValue<U128>; | |
| } | |
| trait FungibleTokenContract { | |
| fn ft_transfer( | |
| receiver_id: AccountId, | |
| amount: U128, | |
| memo: Option<String>, | |
| ); | |
| } |
🤖 Prompt for AI Agents
In contracts/mock-intents/src/lib.rs around lines 18 to 23, the
ext_ft::ft_transfer trait method currently returns PromiseOrValue<U128>, but
NEP-141 (and our mock-ft) expect a unit () return; change the method signature
to return () instead of PromiseOrValue<U128> and update any corresponding
implementations/call sites to return/resolve unit so the promise deserializes
correctly and withdrawals succeed.
| self.internal_withdraw_mt_balance(&sender_id, &token.clone().into(), amount.0); | ||
|
|
||
| ext_ft::ext(token) | ||
| .with_attached_deposit(NearToken::from_yoctonear(1)) | ||
| .ft_transfer(receiver_id, amount, memo) | ||
| .then(Self::ext(env::current_account_id()).on_ft_withdraw(amount)) | ||
| .into() | ||
| } | ||
|
|
||
| #[private] | ||
| pub const fn on_ft_withdraw( | ||
| &mut self, | ||
| amount: U128, | ||
| #[callback_result] call_result: Result<(), PromiseError>, | ||
| ) -> U128 { | ||
| if call_result.is_ok() { | ||
| U128(amount.0) | ||
| } else { | ||
| U128(0) | ||
| } | ||
| } |
There was a problem hiding this comment.
Debit mt_balances only after the FT transfer succeeds
Line 84 removes balance with internal_withdraw_mt_balance before the external ft_transfer resolves, and the callback (lines 94-103) never restores state on failure—it just returns U128(0). Any error (including the serialization bug above or a downstream FT panic) silently burns assets from the vault’s ledger even though no tokens left the FT contract. We need to move the balance mutation into the callback (after confirming call_result.is_ok()) or at least pass sender_id/token_id into the callback and re-credit plus panic on failure so the whole withdraw rolls back. Right now this is a critical correctness breach that can strand user funds.
- self.internal_withdraw_mt_balance(&sender_id, &token.clone().into(), amount.0);
-
- ext_ft::ext(token)
+ let token_id: TokenId = token.clone().into();
+ ext_ft::ext(token)
.with_attached_deposit(NearToken::from_yoctonear(1))
.ft_transfer(receiver_id, amount, memo)
- .then(Self::ext(env::current_account_id()).on_ft_withdraw(amount))
+ .then(
+ Self::ext(env::current_account_id())
+ .on_ft_withdraw(sender_id, token_id, amount),
+ )
.into()
}
#[private]
- pub const fn on_ft_withdraw(
+ pub fn on_ft_withdraw(
&mut self,
+ sender_id: AccountId,
+ token_id: TokenId,
amount: U128,
#[callback_result] call_result: Result<(), PromiseError>,
) -> U128 {
- if call_result.is_ok() {
- U128(amount.0)
- } else {
- U128(0)
- }
+ match call_result {
+ Ok(()) => {
+ self.internal_withdraw_mt_balance(&sender_id, &token_id, amount.0);
+ U128(amount.0)
+ }
+ Err(_) => env::panic_str("FT transfer failed; state reverted"),
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.internal_withdraw_mt_balance(&sender_id, &token.clone().into(), amount.0); | |
| ext_ft::ext(token) | |
| .with_attached_deposit(NearToken::from_yoctonear(1)) | |
| .ft_transfer(receiver_id, amount, memo) | |
| .then(Self::ext(env::current_account_id()).on_ft_withdraw(amount)) | |
| .into() | |
| } | |
| #[private] | |
| pub const fn on_ft_withdraw( | |
| &mut self, | |
| amount: U128, | |
| #[callback_result] call_result: Result<(), PromiseError>, | |
| ) -> U128 { | |
| if call_result.is_ok() { | |
| U128(amount.0) | |
| } else { | |
| U128(0) | |
| } | |
| } | |
| // Defer debiting our internal MT balance until after the FT transfer succeeds. | |
| let token_id: TokenId = token.clone().into(); | |
| ext_ft::ext(token) | |
| .with_attached_deposit(NearToken::from_yoctonear(1)) | |
| .ft_transfer(receiver_id, amount, memo) | |
| .then( | |
| Self::ext(env::current_account_id()) | |
| .on_ft_withdraw(sender_id, token_id, amount), | |
| ) | |
| .into() | |
| } | |
| #[private] | |
| pub fn on_ft_withdraw( | |
| &mut self, | |
| sender_id: AccountId, | |
| token_id: TokenId, | |
| amount: U128, | |
| #[callback_result] call_result: Result<(), PromiseError>, | |
| ) -> U128 { | |
| match call_result { | |
| Ok(()) => { | |
| // Only now debit the MT balance, since the FT transfer succeeded. | |
| self.internal_withdraw_mt_balance(&sender_id, &token_id, amount.0); | |
| U128(amount.0) | |
| } | |
| Err(_) => { | |
| // On failure, panic to revert the entire promise chain and avoid stranded funds. | |
| env::panic_str("FT transfer failed; state reverted") | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In contracts/mock-intents/src/lib.rs around lines 84 to 104, the code debits
mt_balances before the external ft_transfer resolves causing possible permanent
loss on failure; change the flow so the balance mutation happens only after
confirming call_result.is_ok(): remove the pre-transfer
internal_withdraw_mt_balance call, pass the sender_id and token (or token_id)
into the callback (or store them in the promise args), and in on_ft_withdraw, if
call_result.is_ok() then perform internal_withdraw_mt_balance and return the
amount, otherwise re-credit the balance (or do not debit) and panic/return an
error so the withdraw fully rolls back and user funds cannot be silently burned.
| pub fn on_deposit_into_pool( | ||
| &mut self, | ||
| pool_id: u32, | ||
| token_id: &AccountId, | ||
| amount: U128, | ||
| #[callback_result] used_fund: Result<U128, PromiseError>, | ||
| #[callback_result] result: Result<U128, PromiseError>, | ||
| ) -> U128 { | ||
| if let Ok(used_fund) = used_fund { | ||
| // Refund the unused amount. | ||
| // ft_transfer_call() returns the used fund | ||
| U128(amount.0.saturating_sub(used_fund.0)) | ||
| } else { | ||
| amount | ||
| match result { | ||
| Ok(used_fund) => { | ||
| if used_fund.0 > 0 { | ||
| Event::AssetDeposited { | ||
| pool_id, | ||
| token_id, | ||
| amount: &amount, | ||
| } | ||
| .emit(); | ||
| } | ||
|
|
||
| // Refund the unused amount. | ||
| // ft_transfer_call() returns the used fund | ||
| U128(amount.0.saturating_sub(used_fund.0)) | ||
| } | ||
| Err(e) => { | ||
| env::panic_str(&format!("Error depositing into pool: {e:?}")); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Deposit callback emits wrong amount, doesn’t update local accounting, and doesn’t forward refunds
- Event emits the requested amount, not the used amount. It should log used_fund.
- Pool state isn’t updated, so balances drift from the Intents vault.
- Unused tokens are refunded by the FT back to this contract (sender of ft_transfer_call), not to the original caller; you’re returning a number but not transferring the tokens to the user. You already had
_sender_idin the deposit API — use it.
Proposed fix (adjust signatures, update state, and forward unused tokens):
- pub fn on_deposit_into_pool(
+ pub fn on_deposit_into_pool(
&mut self,
- pool_id: u32,
- token_id: &AccountId,
- amount: U128,
+ pool_id: u32,
+ token_id: &AccountId,
+ amount: U128,
+ sender_id: AccountId,
#[callback_result] result: Result<U128, PromiseError>,
) -> U128 {
match result {
- Ok(used_fund) => {
- if used_fund.0 > 0 {
+ Ok(used_fund) => {
+ // Update local pool accounting.
+ if used_fund.0 > 0 {
+ let mut pool = self.pools.get(pool_id).expect(ERR_POOL_NOT_FOUND);
+ if let Some(idx) = pool.token_ids.iter().position(|t| t == token_id) {
+ pool.amounts[idx] = pool.amounts[idx].saturating_add(used_fund.0);
+ self.pools.replace(pool_id, &pool);
+ self.pools.flush();
+ }
Event::AssetDeposited {
- pool_id,
- token_id,
- amount: &amount,
+ pool_id,
+ token_id,
+ amount: &used_fund,
}
.emit();
}
- // Refund the unused amount.
- // ft_transfer_call() returns the used fund
- U128(amount.0.saturating_sub(used_fund.0))
+ // Forward unused amount (already refunded by FT back to this contract) to the original sender.
+ let unused = amount.0.saturating_sub(used_fund.0);
+ if unused > 0 {
+ ext_ft::ext(token_id.clone())
+ .with_attached_deposit(ONE_YOCTO)
+ .ft_transfer(sender_id.clone(), U128(unused), Some("refund unused deposit".to_string()));
+ }
+ U128(unused)
}
Err(e) => {
env::panic_str(&format!("Error depositing into pool: {e:?}"));
}
}
}And pass the sender into the callback:
- pub(crate) fn deposit_into_pool(
- &self,
+ pub(crate) fn deposit_into_pool(
+ &self,
pool_id: u32,
token_id: &AccountId,
- _sender_id: &AccountId,
+ sender_id: &AccountId,
amount: Balance,
) -> PromiseOrValue<U128> {
@@
.then(
Self::ext(env::current_account_id())
.with_static_gas(GAS_DEPOSIT_INTO_POOL_CALLBACK)
.with_unused_gas_weight(0)
- .on_deposit_into_pool(pool_id, token_id, U128(amount)),
+ .on_deposit_into_pool(pool_id, token_id, U128(amount), sender_id.clone()),
)Note: Increase GAS_DEPOSIT_INTO_POOL_CALLBACK as suggested, since we update storage and may forward refunds. Based on learnings.
🏁 Script executed:
#!/bin/bash
# Find all call sites to update callback signature
rg -nP 'on_deposit_into_pool\s*\(' -C2
rg -nP 'deposit_into_pool\s*\(' -C3Length of output: 6403
🏁 Script executed:
#!/bin/bash
rg -nP 'fn deposit_into_pool' -C2 contracts/solver-registry/src/token_receiver.rsLength of output: 0
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' contracts/solver-registry/src/token_receiver.rsLength of output: 1045
🏁 Script executed:
#!/bin/bash
sed -n '175,250p' contracts/solver-registry/src/pool.rsLength of output: 1379
🏁 Script executed:
#!/bin/bash
rg -nP 'pub async fn deposit_into_pool' -C3 contracts/solver-registry/tests/common/utils.rsLength of output: 165
Fix deposit callback to log correct amount, sync state, and refund unused tokens
- In on_deposit_into_pool (pool.rs 145–171): emit used_fund instead of requested amount, update self.pools by adding used_fund.0 to the pool’s token balance (get → modify → replace → flush).
- Extend on_deposit_into_pool signature to take sender_id: AccountId and after computing unused = amount.0 – used_fund.0, call ext_ft::ft_transfer(sender_id, U128(unused), …) to forward the refund.
- In deposit_into_pool (pool.rs 181–210), pass sender_id.clone() into .on_deposit_into_pool(…) instead of only (pool_id, token_id, U128(amount)).
- Bump GAS_DEPOSIT_INTO_POOL_CALLBACK to account for the extra storage writes and the refund transfer.
🤖 Prompt for AI Agents
In contracts/solver-registry/src/pool.rs around lines 145–171, the deposit
callback currently emits the requested amount, does not update pool state with
the actually used_fund, and fails to refund unused tokens; change the
on_deposit_into_pool signature to accept sender_id: AccountId, emit used_fund
(not the original requested amount), update self.pools by retrieving the pool
entry, adding used_fund.0 to the pool’s token balance (modify → replace →
flush), compute unused = amount.0.saturating_sub(used_fund.0) and call
ext_ft::ft_transfer(sender_id, U128(unused), ...) to refund the caller, and
panic only on Err; then update deposit_into_pool (around lines 181–210) to pass
sender_id.clone() into .on_deposit_into_pool(...), and increase
GAS_DEPOSIT_INTO_POOL_CALLBACK to cover the extra storage writes and the refund
transfer.
| // Create a liquidity pool | ||
| create_liquidity_pool(&solver_registry, &wnear, &usdc).await?; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Build is red: include_bytes! for intents_vault.wasm not found
Workspace build fails before tests due to solver-registry/src/pool.rs embedding a missing WASM at:
../../intents-vault/res/intents_vault.wasm.
Two options:
- Quick: commit the WASM and point include_bytes! to an existing path in the repo.
- Robust: add a build.rs in solver-registry to copy/build the intents-vault WASM into a known location and set an env var used by include_bytes!.
Example (robust) in solver-registry:
// src/pool.rs
- .deploy_contract(include_bytes!("../../intents-vault/res/intents_vault.wasm").to_vec())
+ .deploy_contract(include_bytes!(env!("INTENTS_VAULT_WASM_PATH")).to_vec())build.rs:
use std::{env, fs, path::PathBuf};
fn main() {
// Adjust relative paths as needed for your workspace layout
let manifest_dir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap());
let wasm = manifest_dir.join("../intents-vault/res/intents_vault.wasm");
// Fail early with a clear error if missing
if !wasm.exists() {
panic!("Missing intents_vault.wasm at {}", wasm.display());
}
println!("cargo:rustc-env=INTENTS_VAULT_WASM_PATH={}", wasm.display());
}🏁 Script executed:
#!/bin/bash
# Verify presence of the embedded WASM and referenced path(s)
set -euo pipefail
echo "Checking for intents_vault.wasm under workspace..."
fd -a 'intents_vault.wasm'
echo "Grepping include_bytes usage and env var..."
rg -n 'include_bytes!\(' -C2 contracts/solver-registry/src | sed -n '1,120p'Length of output: 191
Please run the following to verify the presence of intents_vault.wasm and any include_bytes! calls:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Listing files in contracts/solver-registry/src:"
find contracts/solver-registry/src -type f
echo
echo "Searching for any reference to intents_vault (.wasm file):"
rg -n 'intents[-_]vault\.wasm' -C2 .
echo
echo "Searching for include_bytes! macro usage in the repo:"
rg -n 'include_bytes!' -C2 .Length of output: 1957
Please run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Contents of contracts/intents-vault/res:"
find contracts/intents-vault/res -maxdepth 1 -type f -printf '%f\n'Length of output: 103
Fix missing intents_vault.wasm import in solver-registry
Build is failing because pool.rs calls
.deploy_contract(include_bytes!("../../intents-vault/res/intents_vault.wasm").to_vec())but contracts/intents-vault/res/intents_vault.wasm doesn’t exist. Choose one:
- Commit the compiled WASM at
contracts/intents-vault/res/intents_vault.wasm. - Add a
build.rsincontracts/solver-registrythat locates/copies the WASM and sets an env var forinclude_bytes!(env!("INTENTS_VAULT_WASM_PATH")).
Location:contracts/solver-registry/src/pool.rs:108
🤖 Prompt for AI Agents
In contracts/solver-registry/tests/test_withdraw_assets.rs around lines 19-21
and specifically addressing contracts/solver-registry/src/pool.rs line 108, the
test fails because pool.rs calls
include_bytes!("../../intents-vault/res/intents_vault.wasm") but that file isn’t
committed; fix by adding a build script in contracts/solver-registry (build.rs)
that locates the compiled intents-vault wasm (e.g.,
../intents-vault/res/intents_vault.wasm or target path), copies or verifies it
into the solver-registry crate (or sets an absolute path), and sets an
environment variable INTENTS_VAULT_WASM_PATH pointing to the wasm file so
pool.rs can use include_bytes!(env!("INTENTS_VAULT_WASM_PATH")); ensure the
build.rs emits rerun-if-changed for the source wasm and that the CI/build
process either checks in the compiled WASM or builds intents-vault before
running solver-registry tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
makefile (2)
3-15: Avoid couplinglintto the intents-vault buildHaving
lintdepend onintents-vaultmeans every formatting or clippy pass now compiles and copies the vault WASM, which slows down feedback and introduces unrelated build breakages into linting. Please drop the dependency somake lintonly runscargo fmt/cargo clippy.
1-33: Declare the phony targets explicitly
all,test, andclean(and the other helper targets) don’t correspond to real files; without a.PHONYstanza they can be skipped if a file with the same name exists. Add a single.PHONY: all lint test clean solver-registry intents-vault mock-intents mock-ftblock near the top to keep them reliable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
makefile(2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 1-1: Target "all" should be declared PHONY.
(phonydeclared)
⏰ 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: Tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/test.yml (1)
21-24: Rename lint step for clarity.The step title still says “Run cargo clippy,” but it now executes
make lint. Please update the name so the workflow log reflects what’s actually happening.Apply this diff:
- - name: Run cargo clippy + - name: Run lint run: make lintcontracts/mock-intents/Cargo.toml (1)
15-17: Image/digest consistency verified across workspaceAll Cargo.toml files with a
[package.metadata.near.reproducible_build]section use the imagesourcescan/cargo-near:0.17.0-rust-1.86.0and digestsha256:1784ca6310f3496f0048356ce420921c8f5fdf71ee8124d43a2e1ceb1f70db8a. No mismatches detected. Consider switching to “reproducible-wasm” incontainer_build_commandfor full NEP-330 determinism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcontracts/intents-vault/res/intents_vault.wasmis excluded by!**/*.wasm
📒 Files selected for processing (6)
.github/workflows/bulid.yml(1 hunks).github/workflows/test.yml(2 hunks)contracts/intents-vault/Cargo.toml(1 hunks)contracts/mock-ft/Cargo.toml(1 hunks)contracts/mock-intents/Cargo.toml(1 hunks)contracts/solver-registry/Cargo.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/bulid.yml
- contracts/intents-vault/Cargo.toml
⏰ 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). (2)
- GitHub Check: Build Solver Registry (solver-registry)
- GitHub Check: Tests
🔇 Additional comments (2)
contracts/solver-registry/Cargo.toml (1)
15-17: Image/digest pin looks proper; keep all crates in lockstep.No issues spotted. Recommend enforcing alignment across contracts and validating the digest in CI.
You can reuse the verification script from the other comment. Optionally add a lightweight CI check that fails if any Cargo.toml has a different image or digest.
contracts/mock-ft/Cargo.toml (1)
15-17: Confirm tag↔digest mapping and repo-wide consistency
- Manually verify that
sourcescan/cargo-near:0.17.0-rust-1.86.0resolves tosha256:1784ca6310f3496f0048356ce420921c8f5fdf71ee8124d43a2e1ceb1f70db8a(e.g. viaskopeo inspect, Docker, or registry API).- Ensure every
contracts/**/Cargo.tomlreproducible_build block pins this exact image and digest.
aleksuss
force-pushed
the
feat/withdraw-from-pool
branch
from
bccfb97 to
71a9018
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/test.yml (4)
17-18: Matrix job looks good; add least-privilege perms and cancel in-progress runs.Keeps both lint/test running; tighten security and CI efficiency with job-level permissions and concurrency.
code-checker: - name: Run ${{ matrix.command }} + name: Run ${{ matrix.command }} + permissions: + contents: read + concurrency: + group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.command }} + cancel-in-progress: true
20-23: Non‑fail‑fast is fine; add a sensible timeout.Prevent hung jobs from burning minutes.
strategy: fail-fast: false matrix: command: [ lint, test ] +timeout-minutes: 30
28-28: Avoid bare curl|sh; verify installer integrity and pin artifact.Pipe‑to‑shell without checksum is risky. Download, verify SHA256, then run. Provide checksum from the release assets.
- - name: Install cargo-near CLI - run: curl --proto '=https' --tlsv1.2 -LsSf https://github.com/near/cargo-near/releases/download/cargo-near-v0.17.0/cargo-near-installer.sh | sh + - name: Install cargo-near CLI + run: | + set -euo pipefail + curl -LsSf \ + -o cargo-near-installer.sh \ + https://github.com/near/cargo-near/releases/download/cargo-near-v0.17.0/cargo-near-installer.sh + # TODO: replace <SHA256SUM> with the value from the release checksum file + echo "<SHA256SUM> cargo-near-installer.sh" | sha256sum -c - + bash cargo-near-installer.shPlease confirm the SHA256 used.
29-30: Ensure Rust toolchain (and wasm target) is installed before make lint/test.Unless your Makefile installs Rust, this will fail on ubuntu‑latest. Install toolchain + rustfmt/clippy + wasm target explicitly.
- name: Checkout repository uses: actions/checkout@v4 + - name: Install Rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt, clippy + targets: wasm32-unknown-unknown - name: Install cargo-near CLI run: | set -euo pipefail curl -LsSf \ -o cargo-near-installer.sh \ https://github.com/near/cargo-near/releases/download/cargo-near-v0.17.0/cargo-near-installer.sh echo "<SHA256SUM> cargo-near-installer.sh" | sha256sum -c - bash cargo-near-installer.sh - name: Run ${{ matrix.command }} run: make ${{ matrix.command }}If your Makefile bootstraps Rust, ignore this and confirm.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcontracts/intents-vault/res/intents_vault.wasmis excluded by!**/*.wasm
📒 Files selected for processing (6)
.github/workflows/bulid.yml(1 hunks).github/workflows/test.yml(1 hunks)contracts/intents-vault/Cargo.toml(1 hunks)contracts/mock-ft/Cargo.toml(1 hunks)contracts/mock-intents/Cargo.toml(1 hunks)contracts/solver-registry/Cargo.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- contracts/mock-ft/Cargo.toml
- contracts/mock-intents/Cargo.toml
- contracts/solver-registry/Cargo.toml
- .github/workflows/bulid.yml
- contracts/intents-vault/Cargo.toml
⏰ 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). (3)
- GitHub Check: Run test
- GitHub Check: Build solver-registry
- GitHub Check: Run lint
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contracts/solver-registry/src/admin.rs (1)
69-72: Consider using Near SDK panic for consistency.The pool lookup uses
env::panic_strwith an inline message. For consistency with the codebase, consider following the pattern suggested in a previous review: use a constant error message (e.g.,ERR_POOL_NOT_FOUNDfrompool.rs) and the Near SDK panic mechanism.Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/solver-registry/src/admin.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/solver-registry/src/admin.rs (3)
contracts/solver-registry/src/pool.rs (2)
new(59-70)get_pool_account_id(175-179)contracts/solver-registry/src/lib.rs (1)
new(93-112)contracts/solver-registry/tests/common/utils.rs (2)
withdraw_from_pool(579-601)get_pool_account_id(533-537)
⏰ 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). (3)
- GitHub Check: Run test
- GitHub Check: Run lint
- GitHub Check: Build solver-registry
🔇 Additional comments (4)
contracts/solver-registry/src/admin.rs (4)
2-10: LGTM!The new imports are appropriate for the cross-contract withdrawal functionality. The callback gas allocation of 5 Tgas is reasonable for handling the withdrawal result.
73-76: LGTM!The validation logic is sound:
- Token ID membership is verified
- Amount is checked for > 0
- The comment appropriately explains why real-time balance checking is skipped
78-96: LGTM!The cross-contract call structure is well-implemented:
- Proper 1 yoctoNEAR deposit attached for the ft_withdraw call
- Callback correctly chained with appropriate gas allocation
- Unused gas weight set to 0 to ensure deterministic gas distribution
- Returns
PromiseOrValue<U128>as specified in the function signature
98-118: LGTM!The callback handler is correctly implemented:
#[private]ensures only the contract can invoke it- Success path emits the
AssetWithdrawnevent after confirming the withdrawal succeeded- Error path panics with a descriptive message, which is appropriate for cross-contract call failures
- Returns the withdrawn amount on success
| impl Contract { | ||
| /// Approve a docker compose hash for worker registration | ||
| /// | ||
| #[payable] |
There was a problem hiding this comment.
Validate attached deposit or remove #[payable] decorator.
The function is marked #[payable] but doesn't validate the attached deposit with assert_one_yocto(). This allows callers to accidentally attach NEAR tokens that would be locked in the contract. Either add assert_one_yocto() at the start of the function (if 1 yoctoNEAR is required for authorization/security), or remove the #[payable] decorator if no deposit is needed.
Apply this diff if 1 yoctoNEAR validation is intended:
#[payable]
#[access_control_any(roles(Role::Owner))]
pub fn approve_compose_hash(&mut self, compose_hash: String) {
+ assert_one_yocto();
DockerComposeHash::try_from_hex(&compose_hash)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[payable] | |
| #[payable] | |
| #[access_control_any(roles(Role::Owner))] | |
| pub fn approve_compose_hash(&mut self, compose_hash: String) { | |
| assert_one_yocto(); | |
| DockerComposeHash::try_from_hex(&compose_hash) | |
| // ... rest of the implementation | |
| } |
🤖 Prompt for AI Agents
In contracts/solver-registry/src/admin.rs at line 15 the function is marked
#[payable] but does not validate attached deposit; either remove the #[payable]
attribute if no deposit is allowed, or keep #[payable] and add
assert_one_yocto() as the first statement in the function body to require a 1
yoctoNEAR deposit for authorization; choose the intended behavior and update the
attribute or insert the assert_one_yocto() call accordingly (ensure the function
imports or has access to assert_one_yocto()).
|
|
||
| /// Remove an approved docker compose has | ||
| /// Remove an approved docker compose hash | ||
| #[payable] |
There was a problem hiding this comment.
Validate attached deposit or remove #[payable] decorator.
Same issue as approve_compose_hash - the function is marked #[payable] but doesn't validate the attached deposit. Add assert_one_yocto() or remove the decorator.
Apply this diff if 1 yoctoNEAR validation is intended:
#[payable]
#[access_control_any(roles(Role::Owner))]
pub fn remove_compose_hash(&mut self, compose_hash: String) {
+ assert_one_yocto();
DockerComposeHash::try_from_hex(&compose_hash)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[payable] | |
| #[payable] | |
| #[access_control_any(roles(Role::Owner))] | |
| pub fn remove_compose_hash(&mut self, compose_hash: String) { | |
| assert_one_yocto(); | |
| DockerComposeHash::try_from_hex(&compose_hash) | |
| // …rest of implementation… | |
| } |
🤖 Prompt for AI Agents
In contracts/solver-registry/src/admin.rs around line 30, the function is marked
#[payable] but does not validate the attached deposit; either remove the
#[payable] attribute or add an assert_one_yocto() check at the start of the
function (or equivalent deposit validation) to enforce a 1 yoctoNEAR attachment;
if you intend to require 1 yocto, insert assert_one_yocto() as the first
statement and keep #[payable], otherwise delete #[payable] to prevent accidental
deposits.
| Event::FullAccessKeyAdded { | ||
| public_key: &public_key, | ||
| } | ||
| .emit(); | ||
| Promise::new(env::current_account_id()).add_full_access_key(public_key) |
There was a problem hiding this comment.
Event emitted before key addition completes.
The FullAccessKeyAdded event is now emitted before the promise to add the key executes. If the promise fails, the event log will incorrectly indicate that the key was added. Consider whether this ordering is intentional, as it creates a potential inconsistency between logged events and actual state.
If the event should only be emitted on success, this requires a callback pattern similar to withdraw_from_pool. Otherwise, document that this event indicates intent rather than completion.
🤖 Prompt for AI Agents
In contracts/solver-registry/src/admin.rs around lines 51 to 55, the
FullAccessKeyAdded event is emitted before the Promise to add the full access
key executes, causing a mismatch if the promise fails; change the flow so the
event is emitted only after the key addition succeeds by converting the current
Promise call into an async callback pattern (similar to withdraw_from_pool):
return a Promise that calls add_full_access_key and then schedules a callback
(e.g., on_add_full_access_key) on success which inspects the callback result and
emits FullAccessKeyAdded only when the result indicates success; alternatively,
if you intend the event to represent intent rather than confirmation, update the
comment/docs at this location to state that the event is emitted optimistically
and may not reflect completion.
3 participants
It's a duplicate of #9
We need a function to withdraw assets from TEE solver's pool back to owner before the adding/removing liquidity pool features are ready.
This is not a problem for the non-TEE AMM solver because the person who runs the AMM solver owns the private key of the solver account, but no one has access to the TEE solver's key pairs. We need to allow the solver registry's owner to withdraw assets from pool when needed.
Summary by CodeRabbit
New Features
Changes
Breaking
Tests
CI/Build