Feat/mt resolve deposit tests #198
Conversation
📝 WalkthroughWalkthroughAdds a raw mt_on_transfer execution path returning full ExecutionFinalResult; introduces a shared Sandbox singleton and fast_forward helper; enforces a 16 KiB event log-size limit for MT refunds; extends multi-token receiver tests with new behaviors and large-return scenarios; adds comprehensive NEP-245 gas-boundary tests and small tooling/formatting tweaks. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Fix all issues with AI agents
In `@tests/src/tests/defuse/tokens/nep245/mod.rs`:
- Around line 17-28: The binary-search loop can underflow when mid == 0 and
test(mid) returns Err; replace the direct subtraction hi = mid - 1 with a
guarded update so that if mid == 0 you break the loop (no lower index exists)
otherwise set hi = mid - 1 (or hi = mid.saturating_sub(1) combined with an if
mid == 0 { break; } check). Update the match arm for Err(_) to use this guard
referencing lo, hi, mid, test and best so the loop cannot wrap to usize::MAX.
🧹 Nitpick comments (4)
tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)
278-291: Test coverage forbinary_search_maxis good but doesn't cover edge case.The test validates all values from 0 to 99, which is comprehensive. However, note that this test only covers cases where at least
test(0)succeeds (sincelimitranges from 0 to 99 andtest(x)succeeds whenx <= limit).Consider adding a test case where all values fail to ensure the underflow issue mentioned in
mod.rsis covered:✨ Additional test case
// Test case where all values fail let test_all_fail = |_x| async { Err(anyhow::anyhow!("always fail")) }; assert_eq!(binary_search_max(0, max, test_all_fail).await, None);sandbox/Cargo.toml (1)
21-21: Add libc to workspace dependencies for consistency.The
libccrate is used in the sandbox code (libc::atexitinsandbox/src/lib.rs) but is specified with a direct version instead of using theworkspace = truepattern like all other dependencies. Declare it in the workspace[dependencies]section to maintain consistency across the codebase.sandbox/src/lib.rs (1)
115-140: Potential cascading test failures on poisoned mutex.If a test panics while the sandbox fixture is being created (before the lock is released), subsequent tests will fail with a panic on line 126's
.unwrap()due to mutex poisoning. Consider recovering from a poisoned mutex or documenting this behavior.That said, for test infrastructure where a panic during setup indicates a fundamental issue, this is often acceptable.
♻️ Optional: Handle poisoned mutex gracefully
let (sandbox_arc, root_account) = mutex .lock() - .unwrap() + .unwrap_or_else(|poisoned| poisoned.into_inner()) .as_ref() .map(|shared| (shared.sandbox.clone(), shared.root.clone())) .unwrap();tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (1)
74-85: Potential underflow in account name length calculation.If
root_id_len >= 63, the subtractionTARGET_LEN - 1 - root_id_lenwill underflow (wrap around to a large value), causing the subaccount creation to likely fail or behave unexpectedly.In practice, the sandbox root ID (
testor similar) is short, so this won't trigger. However, adding a guard would make the code more robust.♻️ Optional: Add a guard for name length calculation
TokenIdGenerationMode::Medium => { // Use a 64-char named account: {name}.{root_id} = 64 chars total const TARGET_LEN: usize = 64; let root_id_len = env.root().id().as_str().len(); // name_len + 1 (dot) + root_id_len = TARGET_LEN + assert!( + root_id_len < TARGET_LEN - 1, + "root_id too long for medium mode" + ); let name_len = TARGET_LEN - 1 - root_id_len;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
sandbox/Cargo.tomlsandbox/src/extensions/mt.rssandbox/src/lib.rssandbox/src/tx/wrappers.rstests/contracts/multi-token-receiver-stub/src/lib.rstests/src/tests/defuse/tokens/nep245/mod.rstests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rstests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs
🧰 Additional context used
🧬 Code graph analysis (3)
tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (3)
tests/src/tests/defuse/tokens/nep245/mod.rs (4)
binary_search_max(8-31)None(996-996)None(1187-1187)None(1558-1558)test-utils/src/random.rs (2)
random(82-84)gen_random_string(121-127)sandbox/src/extensions/mt.rs (2)
tx(114-127)tx(170-179)
tests/src/tests/defuse/tokens/nep245/mod.rs (2)
tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (2)
mt_deposit_resolve_gas(312-389)None(336-336)tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)
mt_transfer_resolve_gas(232-276)
tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)
tests/src/tests/defuse/tokens/nep245/mod.rs (1)
binary_search_max(8-31)
⏰ 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). (10)
- GitHub Check: Build
- GitHub Check: Build Reproducible
- GitHub Check: Security Audit - deny
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (16)
sandbox/src/tx/wrappers.rs (1)
49-58: LGTM!The truncation logic for large byte payloads is well-implemented. The threshold of 32 bytes and showing first/last 16 bytes provides a good balance between readability and debugging utility.
sandbox/src/extensions/mt.rs (1)
184-206: LGTM with minor duplication note.The implementation correctly mirrors
mt_on_transferwhile returning the rawExecutionFinalResult. The code duplication betweenmt_on_transferandmt_on_transfer_raw(lines 165-177 vs 191-203) is acceptable given the different return paths, though a shared helper could reduce this in the future if more*_rawvariants are added.tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)
1-1: LGTM!The import correctly references the refactored
binary_search_maxfrom the parent module. This consolidation reduces code duplication across test files.tests/contracts/multi-token-receiver-stub/src/lib.rs (4)
9-16: LGTM!The raw extern function correctly bypasses SDK serialization to return arbitrary bytes for testing purposes. The conditional compilation to
wasm32is appropriate.
18-32: LGTM!The extension trait provides a clean way to chain the raw return value call onto Promise objects. Using
Gas::from_ggas(0)withGasWeight(1)correctly defers gas allocation to the runtime.
45-58: LGTM!The new enum variants are well-documented and cover important edge cases for testing: legitimate refunds, malicious overflow attempts, and large return payloads.
94-96: LGTM!The
ReturnByteshandler correctly creates a raw byte response via the stub mechanism. Thelen.0 as usizecast is safe here since the test uses values well withinu32range (~4MB), and this runs exclusively on wasm32.sandbox/src/lib.rs (3)
6-20: LGTM!The expanded imports and public re-exports provide appropriate concurrency primitives and convenient access to the extension traits for test consumers.
92-95: LGTM!The
fast_forwardmethod provides a useful wrapper for time-based testing scenarios. Theunwrap()is acceptable for test infrastructure.
97-107: LGTM!The
OnceCell<Mutex<Option<Sandbox>>>pattern correctly enables async initialization while allowing synchronous cleanup viaatexit. UsingOption::take()ensures proper ownership transfer during cleanup, and silently handling a poisoned mutex during shutdown is pragmatic.tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs (6)
1-21: LGTM!The imports and the
TOTAL_LOG_LENGTH_LIMITconstant are appropriate for the test module's purpose.
23-51: LGTM!The
ReceiverTestModeenum provides a clean abstraction overMTReceiverModewith appropriate minimum gas values for each test scenario.
113-149: LGTM!The
make_token_idsfunction correctly generates token IDs of varying lengths. The string slicing is safe because both the index prefix and the random string consist of ASCII characters, ensuring single-byte UTF-8 encoding.
166-199: LGTM!The log size validation is a valuable pre-check that ensures test parameters won't exceed blockchain event log limits before executing the expensive on-chain operations.
207-302: LGTM!The test orchestration function is well-structured with clear separation between recoverable errors (for binary search) and critical failures (assertions for contract bugs). The comment on lines 271-273 clearly explains the rationale for the
assert!vsbaildistinction.
304-389: LGTM!The parametrized test provides comprehensive coverage of token ID generation modes and receiver behaviors. The binary search approach efficiently finds the maximum token count, and the minimum threshold assertion (50 tokens) ensures the contract maintains acceptable capacity.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| while lo <= hi { | ||
| let mid = lo + (hi - lo) / 2; | ||
| match test(mid).await { | ||
| Ok(()) => { | ||
| best = Some(mid); | ||
| lo = mid + 1; // success -> try higher | ||
| } | ||
| Err(_) => { | ||
| hi = mid - 1; // failure -> try lower | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential underflow when mid == 0 and test fails.
If low is 0 and test(0) returns Err, line 25 computes hi = mid - 1 which causes unsigned integer underflow (0 - 1 wraps to usize::MAX). This would cause the loop to continue with invalid bounds.
In the current usage with min_token_count = 1, this isn't triggered, but the function could fail unexpectedly if called with low = 0.
🐛 Proposed fix to handle underflow
match test(mid).await {
Ok(()) => {
best = Some(mid);
lo = mid + 1; // success -> try higher
}
Err(_) => {
- hi = mid - 1; // failure -> try lower
+ if mid == 0 {
+ break;
+ }
+ hi = mid - 1; // failure -> try lower
}
}Alternatively, use saturating_sub:
Err(_) => {
- hi = mid - 1; // failure -> try lower
+ hi = mid.saturating_sub(1); // failure -> try lower
+ if mid == 0 {
+ break;
+ }
}📝 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.
| while lo <= hi { | |
| let mid = lo + (hi - lo) / 2; | |
| match test(mid).await { | |
| Ok(()) => { | |
| best = Some(mid); | |
| lo = mid + 1; // success -> try higher | |
| } | |
| Err(_) => { | |
| hi = mid - 1; // failure -> try lower | |
| } | |
| } | |
| } | |
| while lo <= hi { | |
| let mid = lo + (hi - lo) / 2; | |
| match test(mid).await { | |
| Ok(()) => { | |
| best = Some(mid); | |
| lo = mid + 1; // success -> try higher | |
| } | |
| Err(_) => { | |
| if mid == 0 { | |
| break; | |
| } | |
| hi = mid - 1; // failure -> try lower | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@tests/src/tests/defuse/tokens/nep245/mod.rs` around lines 17 - 28, The
binary-search loop can underflow when mid == 0 and test(mid) returns Err;
replace the direct subtraction hi = mid - 1 with a guarded update so that if mid
== 0 you break the loop (no lower index exists) otherwise set hi = mid - 1 (or
hi = mid.saturating_sub(1) combined with an if mid == 0 { break; } check).
Update the match arm for Err(_) to use this guard referencing lo, hi, mid, test
and best so the loop cannot wrap to usize::MAX.
| /// Shared sandbox instance for test fixtures. | ||
| /// Using `OnceCell<Mutex<Option<...>>>` allows async init and taking ownership in atexit. | ||
| static SHARED_SANDBOX: OnceCell<Mutex<Option<Sandbox>>> = OnceCell::const_new(); | ||
|
|
||
| extern "C" fn cleanup_sandbox() { | ||
| if let Some(mutex) = SHARED_SANDBOX.get() { | ||
| if let Ok(mut guard) = mutex.lock() { | ||
| drop(guard.take()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Very cool! Though, I thought there is a second reason for it not to be killed: #[tokio::test] creates a separate tokio runtime for each test, while
tokio::process::Command::kill_on_drop() (added here) seems to work within a single tokio runtime instance, but I might be wrong here.
PS: wrapping in Option might be redundant due to existence of OnceCell::take()
There was a problem hiding this comment.
tried, but
E0596: cannot borrow immutable static item `SHARED_SANDBOX` as mutable
There was a problem hiding this comment.
tokio::process::Command::kill_on_drop()
could help but since its kept in static variable, the value is never dropped ...
There was a problem hiding this comment.
E0596: cannot borrow immutable static item `SHARED_SANDBOX` as mutable
Okay, then maybe get rid of OnceCell entirely? it seems to be redundant, since Mutex<Option<...>> should work, too.
As for kill_on_drop: since you're touching this part of code, can you also evaluate if tokio_shared_rt would help here?
There was a problem hiding this comment.
oncecell guaranteeds that it will initialize sandbox only once, but yeah since we are only setting it bac to none after tests are finished it should be fine to live without it
There was a problem hiding this comment.
fyi: confirmed that near-sandbox is killed in case of successful execution and on ctrl+c
There was a problem hiding this comment.
actually oncecell is quite helpful here due to get_or_init functionatliy. otherwise you need to manually lock the mutex, then there is async call to create sandbox and you dont want to keep mutex locked between await points so the implementation becomes ugly and complicated IMO. so i decided to keep oncecell since its semantically comaptible with what we want => initialize sanbox only once per test run (binary call)
pityjllk
force-pushed
the
feat/mt-resolve-deposit-tests
branch
from
ec08d28 to
1f2f541
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/contracts/multi-token-receiver-stub/src/lib.rs`:
- Around line 13-19: The stub_return_bytes function currently trusts the
incoming length and allocates based on it, risking huge allocations and silent
truncation; update stub_return_bytes to parse the length using a fallible
conversion (e.g., TryFrom/checked conversion instead of direct cast), enforce a
sensible upper bound (cap length to a constant MAX_RETURN_BYTES), and reject or
early-return (with a clear env::panic or no-op) when the parsed length is
invalid or exceeds the cap to avoid allocation/overflow; apply the same
defensive change to the similar code block referenced at lines 99-101 so both
places use try_from/checked conversion and the MAX_RETURN_BYTES cap before
allocating the Vec or calling env::value_return.
In `@tests/src/tests/defuse/tokens/nep245/mt_deposit_resolve_gas.rs`:
- Around line 203-223: The test currently only checks the resolve receipt when
defuse_outcomes.len() == 2, which can skip failures when there are more receipts
or fewer; change the condition to assert that defuse_outcomes.len() >= 2 and
always inspect the second receipt: obtain resolve_outcome =
defuse_outcomes[1].clone(), call resolve_result = resolve_outcome.into_result(),
and assert resolve_result.is_ok() with a clear error message including
resolve_result.err(); if the assertion for >= 2 fails, fail the test immediately
with a descriptive message about the missing resolve receipt (use symbols
defuse_outcomes, resolve_outcome, resolve_result, execution_result,
env.defuse.id() to locate the code).
♻️ Duplicate comments (1)
tests/src/tests/defuse/tokens/nep245/mod.rs (1)
17-28: Potential underflow whenmid == 0and test fails.If
lois 0 andtest(0)returnsErr, line 25 computeshi = mid - 1which causes unsigned integer underflow (0 - 1wraps tousize::MAX). This would cause the loop to continue with invalid bounds.While current usage with
min_token_count = 1avoids this, the function could fail unexpectedly if called withlow = 0.🐛 Proposed fix to handle underflow
match test(mid).await { Ok(()) => { best = Some(mid); lo = mid + 1; // success -> try higher } Err(_) => { + if mid == 0 { + break; + } hi = mid - 1; // failure -> try lower } }
🧹 Nitpick comments (2)
tests/src/tests/defuse/tokens/nep245/mt_transfer_resolve_gas.rs (1)
395-424: Consider consolidating log size calculation helpers.There's some duplication between
calculate_log_sizes(lines 395-424) andvalidate_mt_batch_transfer_log_size(lines 83-106). Both construct similarMtTransferEventstructures for size calculation.♻️ Suggested consolidation
You could refactor to have a single helper that returns both sizes:
-fn validate_mt_batch_transfer_log_size( - sender_id: &AccountId, - receiver_id: &AccountId, - token_ids: &[String], - amounts: &[u128], -) -> anyhow::Result<usize> { - let mt_transfer_event = MtEvent::MtTransfer(Cow::Owned(vec![MtTransferEvent { - authorized_id: None, - old_owner_id: Cow::Borrowed(receiver_id), - new_owner_id: Cow::Borrowed(sender_id), - token_ids: Cow::Owned(token_ids.to_vec()), - amounts: Cow::Owned(amounts.iter().copied().map(U128).collect()), - memo: Some(Cow::Borrowed("refund")), - }])); - - let longest_transfer_log = mt_transfer_event.to_nep297_event().to_event_log(); - - anyhow::ensure!( - longest_transfer_log.len() <= TOTAL_LOG_LENGTH_LIMIT, - "transfer log will exceed maximum log limit" - ); - - Ok(longest_transfer_log.len()) -} +fn validate_mt_batch_transfer_log_size( + sender_id: &AccountId, + receiver_id: &AccountId, + token_ids: &[String], + amounts: &[u128], +) -> anyhow::Result<usize> { + let (_, refund_size) = calculate_log_sizes(sender_id, receiver_id, token_ids, amounts); + anyhow::ensure!( + refund_size <= TOTAL_LOG_LENGTH_LIMIT, + "transfer log will exceed maximum log limit" + ); + Ok(refund_size) +}sandbox/src/lib.rs (1)
92-94: Consider returningResultfromfast_forwardinstead of panicking.Since this is a public API, avoid
unwrap()so callers can handle errors with context; good to lock this in now while the method is new.♻️ Suggested signature change
- pub async fn fast_forward(&self, blocks: u64) { - self.sandbox.fast_forward(blocks).await.unwrap(); - } + pub async fn fast_forward(&self, blocks: u64) -> anyhow::Result<()> { + self.sandbox.fast_forward(blocks).await.map_err(Into::into) + }
pityjllk
force-pushed
the
feat/mt-resolve-deposit-tests
branch
from
4cf4f66 to
89ea3f2
Compare
| let transfer_event = MtTransferEvent { | ||
| authorized_id: None, | ||
| old_owner_id: sender_id.into(), | ||
| new_owner_id: Cow::Borrowed(receiver_id), | ||
| token_ids: token_ids.into(), | ||
| amounts: amounts.into(), | ||
| memo: memo.map(Into::into), | ||
| }; | ||
| require!( | ||
| transfer_event.refund_log_size() <= TOTAL_LOG_LENGTH_LIMIT, | ||
| "too many tokens: refund log would exceed protocol limit" | ||
| ); | ||
| MtEvent::MtTransfer([transfer_event].as_slice().into()).emit(); |
There was a problem hiding this comment.
Maybe avoid spending gas on serializing twice?
let event_str = MtEvent::MtTransfer(/* ... */)
.to_nep297_event()
.to_event_log();
// check that refund event, if happens, would not exceed log limits
require!(event_str.len() <= TOTAL_LOG_LENGTH_LIMIT - r#","memo":"refund""#.len());
env::log_str(event_str);|
|
||
| // When "long" feature is enabled, run the test for all token counts from 1 to max | ||
| // to ensure the invariant holds for every count, not just the maximum. | ||
| #[cfg(feature = "long")] |
There was a problem hiding this comment.
- Can you please enable this feature in CI?
- Are you sure would it work together with
cargo test --workspace, since not all crates have this feature? Maybe should we enable this feature by default or invert the condition to#[cfg(not(feature = "short-only"))]?
| use std::borrow::Cow; | ||
|
|
||
| /// NEAR protocol limit for log messages (16 KiB) | ||
| pub const TOTAL_LOG_LENGTH_LIMIT: usize = 16384; |
There was a problem hiding this comment.
maybe move it to defuse-near-utils crate?
Summary by CodeRabbit
New Features
Tests
Bug Fixes
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.