Escrow proxy #191
Escrow proxy #191
Conversation
pityjllk
force-pushed
the
feat/bus-proxy
branch
from
03955f8 to
92f5d97
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@escrow-proxy/src/contract/tokens/nep141.rs`:
- Around line 84-88: The fallback for parsing the escrow response currently sets
used to U128(0) (via serde_json::from_slice(...).unwrap_or_else(|_| U128(0)))
which causes a full refund even when the transfer succeeded; change the fallback
to treat the escrow as having consumed the entire original_amount (i.e., return
original_amount wrapped as U128) so no refund is issued on parse failures, and
keep or improve the log message around this code that references the variables
used and original_amount to make intent clear.
♻️ Duplicate comments (2)
escrow-proxy/src/lib.rs (1)
14-17: Return an ownedProxyConfigfrom the external interface.Line 16 returns a reference, which can’t be deserialized for
#[ext_contract]cross‑contract calls. Use an ownedProxyConfigand update the impl/callers accordingly.🛠️ Proposed fix
#[ext_contract(ext_escrow_proxy)] pub trait EscrowProxy { - fn config(&self) -> &ProxyConfig; + fn config(&self) -> ProxyConfig; fn context_hash(&self, context: CondVarContext<'static>) -> CryptoHash;escrow-proxy/src/contract/tokens/nep245.rs (1)
86-91: Avoid full refund on parse failure (double‑spend risk).If the transfer succeeded but parsing fails, refunding everything can credit the sender while escrow keeps the tokens. Prefer a conservative “assume fully consumed” fallback.
🛠️ Proposed fix
PromiseResult::Successful(transferred) => { let transferred: Vec<U128> = serde_json::from_slice(&transferred).unwrap_or_else(|_| { - near_sdk::log!("Failed to parse escrow response, refunding all"); - vec![U128(0); original_amounts.len()] + near_sdk::log!("Failed to parse escrow response, assuming full consumption (no refund)"); + original_amounts.clone() });
| let used: U128 = serde_json::from_slice(&transferred).unwrap_or_else(|_| { | ||
| near_sdk::log!("Failed to parse escrow response, refunding all"); | ||
| U128(0) | ||
| }); | ||
| U128(original_amount.0.saturating_sub(used.0)) |
There was a problem hiding this comment.
Avoid full refund on parse failure after a successful transfer.
Line 84’s fallback sets used to zero, which refunds everything even though the escrow call already succeeded. This can double‑spend. Prefer a “full consumption” fallback.
🛠️ Proposed fix
PromiseResult::Successful(transferred) => {
let used: U128 = serde_json::from_slice(&transferred).unwrap_or_else(|_| {
- near_sdk::log!("Failed to parse escrow response, refunding all");
- U128(0)
+ near_sdk::log!("Failed to parse escrow response, assuming full consumption (no refund)");
+ original_amount
});
U128(original_amount.0.saturating_sub(used.0))
}📝 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.
| let used: U128 = serde_json::from_slice(&transferred).unwrap_or_else(|_| { | |
| near_sdk::log!("Failed to parse escrow response, refunding all"); | |
| U128(0) | |
| }); | |
| U128(original_amount.0.saturating_sub(used.0)) | |
| let used: U128 = serde_json::from_slice(&transferred).unwrap_or_else(|_| { | |
| near_sdk::log!("Failed to parse escrow response, assuming full consumption (no refund)"); | |
| original_amount | |
| }); | |
| U128(original_amount.0.saturating_sub(used.0)) |
🤖 Prompt for AI Agents
In `@escrow-proxy/src/contract/tokens/nep141.rs` around lines 84 - 88, The
fallback for parsing the escrow response currently sets used to U128(0) (via
serde_json::from_slice(...).unwrap_or_else(|_| U128(0))) which causes a full
refund even when the transfer succeeded; change the fallback to treat the escrow
as having consumed the entire original_amount (i.e., return original_amount
wrapped as U128) so no refund is issued on parse failures, and keep or improve
the log message around this code that references the variables used and
original_amount to make intent clear.
92f5d97 to
7cbdf16
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@escrow-proxy/src/contract/tokens/nep141.rs`:
- Around line 54-56: The callback currently panics on failed authorization using
promise_result_bool(0).unwrap_or(false), which leaves deposited tokens stuck;
instead, modify the callback (the function handling the promise result invoked
from ft_on_transfer) to return a refund by returning
PromiseOrValue::Value(amount) when promise_result_bool(0) is false or None, and
only proceed with the success path when it is true; locate the code that checks
promise_result_bool(0) and replace the panic branch with logic that constructs
and returns PromiseOrValue::Value(amount) so the token transfer is refunded to
the sender.
In `@escrow-proxy/src/contract/utils.rs`:
- Around line 57-58: Add a short explanatory comment immediately above the
Promise::new(...).state_init(...) call (the auth_call using
Promise::new(auth_contract_id).state_init(auth_contract_state_init,
NearToken::from_near(0))) stating the zero-deposit assumption: that
deterministic account initialization with NearToken::from_near(0) is only safe
if the deployed contract code and initial state size fit within zero-balance
limits or the storage costs are pre-funded (e.g., for per_fill_contract_id or
similar accounts), and include a TODO or reference for verifying contract
size/storage coverage if that invariant changes. Ensure the comment references
auth_contract_id/auth_contract_state_init and NearToken::from_near(0) so future
maintainers understand the assumption and where to check it.
In `@near-utils/src/promise.rs`:
- Around line 14-15: The constants MAX_BOOL_JSON_LEN and MAX_U128_JSON_LEN
currently include array bracket padding but NEAR returns bare serialized values;
update these constants to match the actual serialized lengths (e.g., length of
"false"/"true" and the quoted u128 string without surrounding " [ " and " ] ")
or alternatively add a clear comment explaining the intentional extra padding;
locate MAX_BOOL_JSON_LEN and MAX_U128_JSON_LEN in near-utils::promise.rs and
adjust them to align with how promise_result_bool and promise_result_U128
deserialize raw bytes (or document why the padding is kept for defensive
buffering).
♻️ Duplicate comments (1)
Cargo.toml (1)
144-148: Remove the patch now that upstream PR#104is merged.Per a previous review, the upstream
near-api-rsPR#104has been merged. The comment indicates this should be removed oncenear-api 0.8.3is released. Please verify if version 0.8.3 is available and remove this patch section accordingly.near-api-rs crate latest version crates.io 2026
🧹 Nitpick comments (2)
escrow-proxy/src/contract/utils.rs (2)
3-3: Remove unused import.
promise_result_boolis imported but not used in this file.🧹 Suggested fix
-use defuse_near_utils::promise_result_bool;
29-32: Consider documenting the unwrap rationale.
ContractStorage::init_state(state).unwrap()will panic on serialization failure. While this is likely intentional for a bug scenario, a brief comment or using.expect()with a message would clarify intent.📝 Suggested improvement
StateInit::V1(StateInitV1 { code: self.config.per_fill_contract_id.clone(), - data: ContractStorage::init_state(state).unwrap(), + data: ContractStorage::init_state(state) + .expect("CondVarStateInit serialization should not fail"), })
| if !promise_result_bool(0).unwrap_or(false) { | ||
| near_sdk::env::panic_str("Authorization failed or timed out, refunding"); | ||
| } |
There was a problem hiding this comment.
Panic doesn't trigger a refund; tokens may become stuck.
The panic message claims "refunding" but panicking in a callback doesn't automatically refund the tokens. At this point, the tokens are already deposited into the proxy contract (from the original ft_on_transfer). A panic here leaves tokens stuck in the proxy with no recovery path.
Consider returning PromiseOrValue::Value(amount) to signal a full refund instead of panicking:
🔧 Proposed fix
pub fn check_authorization_and_forward_ft(
&self,
token_contract: AccountId,
escrow_address: AccountId,
amount: U128,
msg: String,
) -> PromiseOrValue<U128> {
- if !promise_result_bool(0).unwrap_or(false) {
- near_sdk::env::panic_str("Authorization failed or timed out, refunding");
- }
+ if !promise_result_bool(0).unwrap_or(false) {
+ near_sdk::log!("Authorization failed or timed out, refunding full amount");
+ return PromiseOrValue::Value(amount);
+ }📝 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.
| if !promise_result_bool(0).unwrap_or(false) { | |
| near_sdk::env::panic_str("Authorization failed or timed out, refunding"); | |
| } | |
| pub fn check_authorization_and_forward_ft( | |
| &self, | |
| token_contract: AccountId, | |
| escrow_address: AccountId, | |
| amount: U128, | |
| msg: String, | |
| ) -> PromiseOrValue<U128> { | |
| if !promise_result_bool(0).unwrap_or(false) { | |
| near_sdk::log!("Authorization failed or timed out, refunding full amount"); | |
| return PromiseOrValue::Value(amount); | |
| } |
🤖 Prompt for AI Agents
In `@escrow-proxy/src/contract/tokens/nep141.rs` around lines 54 - 56, The
callback currently panics on failed authorization using
promise_result_bool(0).unwrap_or(false), which leaves deposited tokens stuck;
instead, modify the callback (the function handling the promise result invoked
from ft_on_transfer) to return a refund by returning
PromiseOrValue::Value(amount) when promise_result_bool(0) is false or None, and
only proceed with the success path when it is true; locate the code that checks
promise_result_bool(0) and replace the panic branch with logic that constructs
and returns PromiseOrValue::Value(amount) so the token transfer is refunded to
the sender.
| let auth_call = Promise::new(auth_contract_id) | ||
| .state_init(auth_contract_state_init, NearToken::from_near(0)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
NEAR state_init account creation minimum balance deposit requirement
💡 Result:
Short answer: When creating an account via state_init (or DeterministicStateInit), the transaction must attach a deposit large enough to cover the account's refundable storage staking — i.e., storage_price_per_byte × number_of_bytes for the account’s initial state (keys, data, and code). You can query the live storage price from the protocol-config RPC endpoint; the docs note a typical storage cost (example) of 0.0001 NEAR/byte which results in ≈0.00182 NEAR for a minimal account with one access key. Any excess is refunded; if the deposit is too small the initialization fails. [1][2]
References:
- NEAR docs — accounts / storage (storage price per byte and example minimal balance). [1]
- Nearcore / Nomicon — deterministic account (state_init) creation requires attaching sufficient deposit to cover account storage; anything above the required amount is refunded. [2]
🏁 Script executed:
# Search for auth_contract_id to understand its creation/funding context
rg "auth_contract_id" --type rust -B 5 -A 5Repository: near/intents
Length of output: 951
🏁 Script executed:
# Search for any funding or initialization of the auth contract
rg "auth_contract" --type rust | head -30Repository: near/intents
Length of output: 3182
🏁 Script executed:
# Look at more context around lines 57-58
cat -n escrow-proxy/src/contract/utils.rs | sed -n '40,75p'Repository: near/intents
Length of output: 1120
🏁 Script executed:
# Search for state_init usage in the codebase to see if there are other examples
rg "state_init" --type rust -B 2 -A 2Repository: near/intents
Length of output: 33681
🏁 Script executed:
# Check the oneshot_condvar contract to understand how auth_contract is handled
cat -n oneshot-condvar/src/contract/mod.rs | head -100Repository: near/intents
Length of output: 4004
🏁 Script executed:
# Search for comments about state_init and zero deposit
rg "state_init" --type rust -B 3 -A 3 | grep -A 3 -B 3 "ZERO\|zero\|deposit"Repository: near/intents
Length of output: 3299
🏁 Script executed:
# Look at the specific context around Promise.state_init calls
cat -n defuse/src/contract/tokens/nep245/core.rs | sed -n '190,210p'Repository: near/intents
Length of output: 987
🏁 Script executed:
# Check if there's documentation on NEP-616 or how state_init handles deposits
rg "NEP.*616\|state_init.*deposit\|storage.*cost" --type rust -B 2 -A 2 | head -50Repository: near/intents
Length of output: 38
🏁 Script executed:
# Look more carefully at the call flow - check if there's any transfer or prefunding before calling cv_wait
cat -n escrow-proxy/src/contract/utils.rs | sed -n '35,75p'Repository: near/intents
Length of output: 1355
🏁 Script executed:
# Search for how deterministic accounts work and if there's special handling
rg "derive_account_id" --type rust -B 5 -A 5 | head -80Repository: near/intents
Length of output: 6182
🏁 Script executed:
# Check if the oneshot_condvar contract expects to be pre-funded
rg "minimum.*balance\|storage.*requirement\|deposit.*require" --type rustRepository: near/intents
Length of output: 38
🏁 Script executed:
# Search for NEP-616 documentation or comments in code
rg "616\|DeterministicStateInit\|state_init.*action" --type rust -B 2 -A 2 | head -60Repository: near/intents
Length of output: 38
🏁 Script executed:
# Look for the actual oneshot_condvar calls to see if there's token transfer or funding involved
rg "cv_wait" --type rust -B 10 -A 5Repository: near/intents
Length of output: 25812
🏁 Script executed:
# Look at the actual flow - check if tokens are transferred as part of cv_wait path
cat -n escrow-proxy/src/contract/tokens/nep245.rs | sed -n '1,150p'Repository: near/intents
Length of output: 3700
🏁 Script executed:
# Search for references or issues about state_init zero deposit
rg "storage.*init\|deterministic.*deposit" --type rust -i | head -40Repository: near/intents
Length of output: 38
🏁 Script executed:
# Check the NEAR SDK docs or comments about NEP-616
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "state_init\|NEP.*616" {} \;Repository: near/intents
Length of output: 79
🌐 Web query:
NEAR protocol state_init zero deposit deterministic account NEP-616
💡 Result:
Short answer — Yes: NEAR added deterministic accounts (IDs prefixed "0s") created via a new DeteministicStateInit action (state_init-like) that may be initialized with zero deposit if the code+initial storage fit below the protocol's zero-balance limit; otherwise the action carries a balance to cover required storage (excess refunded). Deterministic IDs are derived from the Borsh-encoded initial state (keccak256 → hex, prefixed "0s"). See the Nomicon / DataStructures doc and the nearcore release notes for the protocol change. [1][2]
Sources:
- Nomicon: Accounts / Deterministic account (DeterministicStateInit, id derivation, zero-balance behavior). [1]
- nearcore release notes / PRs: protocol upgrade introducing deterministic accounts and new host functions (DeterministicStateInit action). [2]
Clarify the zero-deposit assumption for deterministic account initialization.
The state_init call with NearToken::from_near(0) is valid under NEAR's deterministic account protocol if the contract code and initial state fit within the zero-balance limit. However, this assumption should be documented in a comment to clarify that:
- The contract deployed (
per_fill_contract_id) is small enough to initialize with zero NEAR, or - The storage is otherwise guaranteed to be covered
Without this clarification, future maintainers may incorrectly assume insufficient deposit is a bug.
🤖 Prompt for AI Agents
In `@escrow-proxy/src/contract/utils.rs` around lines 57 - 58, Add a short
explanatory comment immediately above the Promise::new(...).state_init(...) call
(the auth_call using
Promise::new(auth_contract_id).state_init(auth_contract_state_init,
NearToken::from_near(0))) stating the zero-deposit assumption: that
deterministic account initialization with NearToken::from_near(0) is only safe
if the deployed contract code and initial state size fit within zero-balance
limits or the storage costs are pre-funded (e.g., for per_fill_contract_id or
similar accounts), and include a TODO or reference for verifying contract
size/storage coverage if that invariant changes. Ensure the comment references
auth_contract_id/auth_contract_state_init and NearToken::from_near(0) so future
maintainers understand the assumption and where to check it.
| token_ids, | ||
| amounts.clone(), | ||
| None, | ||
| Some("proxy".to_string()), |
There was a problem hiding this comment.
unify forward memos, please
| ) | ||
| .then( | ||
| Self::ext(env::current_account_id()) | ||
| .with_static_gas(Gas::from_tgas(10)) |
There was a problem hiding this comment.
| .with_static_gas(Gas::from_tgas(10)) | |
| .with_static_gas(Gas::from_tgas(10)) | |
| .with_unused_gas_weight(0) |
| pub fn check_authorization_and_forward_mt( | ||
| &self, | ||
| token_contract: AccountId, | ||
| escrow_address: AccountId, |
There was a problem hiding this comment.
| escrow_address: AccountId, | |
| receiver_id: AccountId, |
| #[private] | ||
| pub fn check_authorization_and_forward_mt( | ||
| &self, | ||
| token_contract: AccountId, |
There was a problem hiding this comment.
| token_contract: AccountId, | |
| token: AccountId, |
| let used = promise_result_vec_U128(0, original_amounts.len()).unwrap_or_default(); | ||
|
|
||
| original_amounts | ||
| .iter() | ||
| .zip(used.iter()) |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@oneshot-condvar/README.md`:
- Around line 58-130: The README's fenced code blocks that show state-transition
diagrams lack a language specifier and should be marked (e.g., ```text) so they
render correctly; update each diagram block containing the sequences with
cv_wait(), cv_notify_one(), cv_wait_resume(), Authorized,
WaitingForNotification, Idle, Done, Timeout etc. to start with ```text (apply to
all four diagram blocks shown) and keep the diagram content unchanged.
In `@tests/src/tests/oneshot_condvar/mod.rs`:
- Around line 298-299: The test currently only checks JSON parsing for the
response held in the authorized variable
(assert!(authorized.unwrap().json::<bool>().is_ok())) but not the boolean value
itself; change the assertion to parse the JSON and assert the boolean is false
(e.g., unwrap the response, call json::<bool>().unwrap() and assert_eq! or
assert! that the value is false) for the authorized response; apply the same
change to the other occurrence around lines 368-369 to ensure the timeout result
is actually false.
- Around line 180-182: Replace the fixed wall-clock sleep
(tokio::time::sleep(Duration::from_secs(3)).await) with advancing the test
sandbox/time driver so the test is deterministic; e.g., ensure the test enables
paused time (tokio::time::pause() or the test harness equivalent) and call
tokio::time::advance(Duration::from_secs(3)).await (or the harness's
advance_time method) at the same spot where tokio::time::sleep(...) is now to
simulate passage of time without real waiting.
♻️ Duplicate comments (1)
tests/src/tests/escrow_with_proxy/swap.rs (1)
207-216: Configuration explanation is clear, but consider using a dedicated condvar contract.The inline comment explains why
escrow_swap_globalis used forper_fill_contract_idin the cancel test. While functionally correct (cancel doesn't use this field), usingcondvar_globalwould be more semantically accurate and consistent with the full flow test.This is acceptable as-is given the explanation, but for test maintainability, consider deploying
condvar_globalhere too.
🧹 Nitpick comments (9)
tests/src/tests/escrow_with_proxy/swap_with_fees.rs (2)
190-192: Consider adding a gas threshold assertion for regression detection.This is labeled as a gas benchmark test, but it only prints the gas consumed without asserting against any threshold. Consider adding an upper bound assertion to detect gas regressions in CI:
const MAX_EXPECTED_GAS: Gas = Gas::from_tgas(/* expected max TGas */); assert!( total_gas <= MAX_EXPECTED_GAS, "Gas regression: {total_gas:?} exceeds threshold {MAX_EXPECTED_GAS:?}" );This would help catch unexpected gas increases in the proxy flow.
194-212: Strengthen assertions to validate fee distribution.The test configures protocol and integrator fees as part of the "worst-case" scenario but only verifies that maker and proxy received tokens. Consider also verifying that
fee_collectorandintegratorreceived their respective fees to confirm the fee paths were actually exercised:♻️ Suggested additional assertions
// Verify fee collector received protocol fee assert!( env.defuse .mt_balance_of(fee_collector.id(), &token_b_defuse_id.to_string()) .await .unwrap() > 0, "Fee collector should have received protocol fees" ); // Verify integrator received integrator fee assert!( env.defuse .mt_balance_of(integrator.id(), &token_b_defuse_id.to_string()) .await .unwrap() > 0, "Integrator should have received integrator fees" );This ensures the gas measurement truly reflects the worst-case scenario where all fee transfer paths are executed.
oneshot-condvar/src/contract/mod.rs (2)
142-151:cv_notify_oneis#[payable]without deposit handling.Same consideration as
on_auth- if deposits shouldn't be accepted, either remove#[payable]or add explicit rejection.
117-127: Early return inIdlebranch obscures control flow.The
returnon line 126 inside the match arm makes it less obvious that thePromiseOrValue::Value(...)at line 139 only executes for theAuthorizedbranch. Consider restructuring:♻️ Suggested refactor for clarity
match state.state { StateMachine::Idle => { let (promise, yield_id) = Promise::new_yield( "cv_wait_resume", EMPTY_JSON, Gas::from_tgas(0), GasWeight(1), ); state.state = StateMachine::WaitingForNotification(yield_id); - return promise.into(); + promise.into() } StateMachine::Authorized => { state.state = StateMachine::Done; + PromiseOrValue::Value(true) } StateMachine::WaitingForNotification(_) => { env::panic_str("already waiting for notification"); } StateMachine::Done => { env::panic_str("already done"); } } - - PromiseOrValue::Value(matches!(state.state, StateMachine::Done))oneshot-condvar/README.md (2)
105-117: Consider adding sections for initialization, access control, and operational details.The README would benefit from additional sections covering:
- Initialization: Parameters required when deploying the contract (authorizee account, notifier_id account, timeout duration)
- Access Control: Which accounts can call
cv_wait()vscv_notify_one()- Cleanup Details: What happens to the contract's remaining balance when it self-destructs
- Gas Costs: Expected gas consumption for each operation to help users plan their transactions
These details would help users understand how to properly deploy and interact with the contract.
85-95: Consider clarifying the race condition scenario.The race condition path (Path 4) describes an important edge case where notification arrives after yield timeout but before the timeout callback executes. While technically accurate, the explanation could be clearer:
- Line 92: "Yield times out internally" could specify this means "the NEAR yield promise times out in the runtime"
- Consider adding a note explaining why this behavior is desirable: it prevents unnecessary retries when notification arrives during the timeout window
- The relationship between yield timeout and callback execution timing could be made more explicit
This would help readers understand this subtle but important aspect of the contract's behavior.
tests/src/tests/escrow/swaps.rs (1)
3-3: Prefer deterministic ordering for whitelisted accounts.
HashSetiteration is nondeterministic (Line 210), and its iterator feedsParamsBuilder; if ordering affects param serialization or escrow IDs, tests can become flaky. ConsiderBTreeSet(or sortedVec) for stable ordering.♻️ Suggested deterministic ordering
-use std::collections::{BTreeMap, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap};- let mut all_accounts: HashSet<&'static str> = + let mut all_accounts: BTreeSet<&'static str> = test_case.fills.iter().map(|(name, _)| *name).collect();- let unique_takers: HashSet<_> = test_case.fills.iter().map(|(name, _)| *name).collect(); + let unique_takers: BTreeSet<_> = test_case.fills.iter().map(|(name, _)| *name).collect();Also applies to: 210-266
tests/src/tests/escrow_proxy/proxy_oneshot_condvar.rs (2)
21-31: Prefer an explicit error message on init_state failure.
unwrap()here will hide the failure context if serialization ever breaks; a shortexpectmakes test failures easier to diagnose.♻️ Suggested tweak
- let raw_state = ContractStorage::init_state(state.clone()).unwrap(); + let raw_state = ContractStorage::init_state(state.clone()) + .expect("oneshot-condvar state_init serialization should succeed");
79-89: Avoid hardcoded timeout window in fast_forward.
fast_forward(250)will go stale if the proxy’s auth timeout changes. Consider pulling the timeout from a shared constant/config so this test remains aligned.
| ``` | ||
| Idle ──cv_wait()──► WaitingForNotification ──cv_notify_one()──► Authorized ──cv_wait_resume()──► Done | ||
| ``` | ||
|
|
||
| 1. `cv_wait()`: Creates yield promise, state → `WaitingForNotification` | ||
| 2. `cv_notify_one()`: Resumes yield, state → `Authorized` | ||
| 3. `cv_wait_resume()` callback: State → `Done`, returns `true` | ||
|
|
||
| ### Path 2: cv_wait() Timeout | ||
|
|
||
| ``` | ||
| Idle ──cv_wait()──► WaitingForNotification ──timeout──► Idle (can retry) | ||
| ``` | ||
|
|
||
| 1. `cv_wait()`: Creates yield promise, state → `WaitingForNotification` | ||
| 2. Yield times out | ||
| 3. `cv_wait_resume()` callback: State → `Idle`, emits `Timeout`, returns `false` | ||
|
|
||
| ### Path 3: cv_notify() then cv_wait() (Notify first) | ||
|
|
||
| ``` | ||
| Idle ──cv_notify_one()──► Authorized ──cv_wait()──► Done | ||
| ``` | ||
|
|
||
| 1. `cv_notify_one()`: State → `Authorized` | ||
| 2. `cv_wait()`: Immediate success, state → `Done`, returns `true` | ||
|
|
||
| ### Path 4: Race Condition (Timeout + Late Notification) | ||
|
|
||
| ``` | ||
| Idle ──cv_wait()──► WaitingForNotification ──(yield times out)──► cv_notify_one() ──► Authorized ──cv_wait_resume()──► Done | ||
| ``` | ||
|
|
||
| 1. `cv_wait()`: State → `WaitingForNotification` | ||
| 2. Yield times out internally (callback not yet executed) | ||
| 3. `cv_notify_one()` arrives: `yield.resume()` fails, but state → `Authorized` | ||
| 4. `cv_wait_resume()` callback: Sees `Authorized` → `Done`, returns `true` | ||
|
|
||
| ## Error Conditions | ||
|
|
||
| | Current State | Method | Error | | ||
| |---------------|--------|-------| | ||
| | `WaitingForNotification` | `cv_wait()` | "already waiting for notification" | | ||
| | `Done` | `cv_wait()` | "already done" | | ||
| | `Authorized` | `cv_notify_one()` | "already notified" | | ||
| | `Done` | `cv_notify_one()` | "already notified" | | ||
|
|
||
| ## API | ||
|
|
||
| ### `cv_wait() -> PromiseOrValue<bool>` | ||
| Called by the `authorizee` to wait for notification. Returns: | ||
| - `true` if notification received (state becomes `Done`) | ||
| - `false` if timeout occurred (state resets to `Idle`, can retry) | ||
|
|
||
| ### `cv_notify_one()` | ||
| Called by the `notifier_id` to signal notification. Wakes up any waiting `cv_wait()`. | ||
|
|
||
| ### `cv_is_notified() -> bool` | ||
| Returns `true` if state is `Authorized` or `Done`. | ||
|
|
||
| ## Usage Pattern | ||
|
|
||
| ``` | ||
| Party A (authorizee) Contract Party B (notifier_id) | ||
| | | | | ||
| |------- cv_wait() ------>| | | ||
| | [WaitingForNotification] | | ||
| | |<--- cv_notify_one() ----| | ||
| | [Authorized] | | ||
| |<-- cv_wait_resume() ----| | | ||
| | returns true [Done] | | ||
| | [cleanup] | | ||
| ``` |
There was a problem hiding this comment.
Add language specifiers to fenced code blocks.
The fenced code blocks showing state transition flows should have a language specified for proper rendering. Consider using text for the state flow diagrams.
📝 Suggested fix
-```
+```text
Idle ──cv_wait()──► WaitingForNotification ──cv_notify_one()──► Authorized ──cv_wait_resume()──► Done
Apply similar changes to the code blocks at lines 68, 78, 87, and 120.
</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
78-78: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
---
120-120: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
In @oneshot-condvar/README.md around lines 58 - 130, The README's fenced code
blocks that show state-transition diagrams lack a language specifier and should
be marked (e.g., ```text) so they render correctly; update each diagram block
containing the sequences with cv_wait(), cv_notify_one(), cv_wait_resume(),
Authorized, WaitingForNotification, Idle, Done, Timeout etc. to start with
unchanged.
| // replace with waiting for couple blocks | ||
| tokio::time::sleep(Duration::from_secs(3)).await; | ||
|
|
There was a problem hiding this comment.
Avoid fixed sleeps to reduce test flakiness.
Use sandbox time advancement instead of wall-clock sleep so the test remains deterministic and faster.
🔧 Suggested change
- tokio::time::sleep(Duration::from_secs(3)).await;
+ sandbox.fast_forward(5).await;🤖 Prompt for AI Agents
In `@tests/src/tests/oneshot_condvar/mod.rs` around lines 180 - 182, Replace the
fixed wall-clock sleep (tokio::time::sleep(Duration::from_secs(3)).await) with
advancing the test sandbox/time driver so the test is deterministic; e.g.,
ensure the test enables paused time (tokio::time::pause() or the test harness
equivalent) and call tokio::time::advance(Duration::from_secs(3)).await (or the
harness's advance_time method) at the same spot where tokio::time::sleep(...) is
now to simulate passage of time without real waiting.
| assert!(authorized.unwrap().json::<bool>().is_ok()); | ||
|
|
There was a problem hiding this comment.
Assert the timeout result is actually false.
These assertions currently only validate JSON parsing. A true result would still pass.
✅ Suggested change
- assert!(authorized.unwrap().json::<bool>().is_ok());
+ assert!(!authorized.unwrap().json::<bool>().unwrap());- assert!(authorized.unwrap().json::<bool>().is_ok());
+ assert!(!authorized.unwrap().json::<bool>().unwrap());Also applies to: 368-369
🤖 Prompt for AI Agents
In `@tests/src/tests/oneshot_condvar/mod.rs` around lines 298 - 299, The test
currently only checks JSON parsing for the response held in the authorized
variable (assert!(authorized.unwrap().json::<bool>().is_ok())) but not the
boolean value itself; change the assertion to parse the JSON and assert the
boolean is false (e.g., unwrap the response, call json::<bool>().unwrap() and
assert_eq! or assert! that the value is false) for the authorized response;
apply the same change to the other occurrence around lines 368-369 to ensure the
timeout result is actually false.
Summary
This PR introduces a bus proxy architecture for atomic cross-contract token swaps
New Crates:
Escrow-Swap Enhancements:
Testing:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.