Refactor/test separation #190
Conversation
📝 WalkthroughWalkthroughThis PR consolidates test utilities and contract extensions into a new workspace crate Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 @defuse/src/contract/accounts/account/entry/tests/legacy.rs:
- Line 12: The remaining files still import the old symbol `use
defuse_test_utils::random::make_arbitrary;`; replace those imports with `use
defuse_tests::utils::random::make_arbitrary;` everywhere (e.g., in modules that
call `make_arbitrary()` such as NEP171/NEP141/NEP245/token-id related tests) so
all tests consistently use the consolidated `defuse_tests` crate.
🧹 Nitpick comments (10)
tests/tests/defuse/tokens/nep141.rs (2)
22-29: Well-structured expectation type for parameterized testing.The
TransferCallExpectationstruct cleanly encapsulates test case configuration. One observation:intent_transfer_amountisNonein all current test cases (lines 32-66), meaning theExecuteaction path (lines 125-133) is never exercised.Consider adding test cases with
intent_transfer_amount: Some(...)to cover the intent execution path if that's in scope for this PR.
53-58: Minor: Inconsistent number literal formatting.Lines 36-37 use
1_000with underscores for readability, while lines 57-58 and 64 use1000without. Consider using consistent formatting throughout.🔎 Suggested fix
#[case::receiver_panics_results_with_no_refund(TransferCallExpectation { action: StubAction::Panic, intent_transfer_amount: None, refund_if_fails: true, - expected_sender_ft_balance: 1000, + expected_sender_ft_balance: 1_000, expected_receiver_mt_balance: 0, })] #[case::malicious_receiver(TransferCallExpectation { action: StubAction::MaliciousReturn, intent_transfer_amount: None, refund_if_fails: true, - expected_sender_ft_balance: 1000, + expected_sender_ft_balance: 1_000, expected_receiver_mt_balance: 0, })]tests/tests/defuse/tokens/nep171.rs (2)
63-64: Consider moving imports to module level.These imports are placed inside the function body. While valid Rust, it's more conventional to place imports at the module level for better visibility and consistency with the rest of the file.
🔎 Suggested refactor
Move these imports to the top of the file with other imports:
use defuse::core::token_id::TokenId as DefuseTokenId; use defuse::core::token_id::nep171::Nep171TokenId; use defuse::tokens::{DepositAction, DepositMessage, ExecuteIntents}; +use defuse::core::{amounts::Amounts, intents::tokens::Transfer}; +use defuse_sandbox::{api::types::json::Base64VecU8, tx::FnCallBuilder}; use defuse_sandbox::api::types::nft::NFTContractMetadata;And later (line 144):
let deposit_message = if intents.is_empty() { - use defuse::core::intents::tokens::NotifyOnTransfer; + // NotifyOnTransfer should be imported at module level
20-28: Test cases cover notification path only.Similar to the NEP-141 tests, all cases have
intent_transfer: false, so theExecuteaction path (lines 152-159) is never exercised. The notification path scenarios are well-covered.Consider adding at least one case with
intent_transfer: trueto verify the intent execution flow works correctly for NFTs.Also applies to: 30-58
defuse/tests/mod.rs (1)
1-1: Consider more selective dead code suppression.The blanket
#![allow(dead_code)]at the file level may mask genuinely unused code. Consider applying the attribute only to specific items that are intentionally unused (e.g., shared test helpers used conditionally across submodules).Alternative approach
Remove the file-level attribute and apply
#[allow(dead_code)]only to specific items:-#![allow(dead_code)] - mod accounts; mod intents; mod state; mod storage; mod tokens;Then add
#[allow(dead_code)]annotations to specific functions or items within the submodules that need them.tests/tests/defuse/tokens/nep245.rs (1)
73-73: Redundant inner import.
AmountsandTransferare already imported at file scope (lines 2 and 4). This innerusestatement can be removed.🔎 Proposed fix
async fn mt_transfer_call_calls_mt_on_transfer_single_token( #[case] expectation: MtTransferCallExpectation, ) { - use defuse::core::{amounts::Amounts, intents::tokens::Transfer}; - let env = Env::builder().deployer_as_super_admin().build().await;escrow-swap/tests/helpers/mod.rs (1)
1-11: Address the TODO comment or remove if no longer applicable.The import reorganization under
defuse_contract_extensionsis consistent with the PR's objective. However, Line 3 contains aTODO: reorganize importscomment that suggests the structure may need further refinement.Are these imports in their final structure, or is additional reorganization planned? If the current structure is acceptable, consider removing the TODO comment to avoid confusion.
escrow-swap/Cargo.toml (1)
22-22: Clarify if optional runtime dependency is needed.
defuse-sandboxis declared both as an optional runtime dependency (line 22) and as a dev-dependency (line 37). If sandbox is only used for testing, the optional runtime dependency may be unnecessary. Consider removing line 22 unless there's a feature flag that enables sandbox functionality in production builds.tests/tests/defuse/intents.rs (1)
190-192: Remove redundant imports.These imports duplicate what's already declared at the top of the file (lines 9-10 for
ExecuteIntentsExt, line 14 forNearTokenvianear_sdk). TheFnCallBuilderimport fromdefuse_sandbox::txmay be the only new one needed.🔎 Proposed fix
async fn transfer_intent_with_msg_to_receiver_smc(#[case] expectation: TransferCallExpectation) { - use defuse_contract_extensions::defuse::intents::ExecuteIntentsExt; use defuse_sandbox::tx::FnCallBuilder; - use near_sdk::NearToken;defuse/tests/tokens/nep245/mod.rs (1)
690-720: Consider documenting or removing the gas measurement scaffolding.The commented-out gas measurement code provides useful debugging capability but clutters the test. Consider either:
- Moving to a separate utility/helper that can be enabled via feature flag
- Adding a brief comment explaining its purpose for future maintainers
- Removing if no longer needed
As-is, the scaffolding is harmless but adds cognitive overhead.
📜 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 (76)
Cargo.tomlcontract_extensions/Cargo.tomlcontract_extensions/src/defuse/account_manager.rscontract_extensions/src/defuse/deployer.rscontract_extensions/src/defuse/force_manager.rscontract_extensions/src/defuse/intents.rscontract_extensions/src/defuse/mod.rscontract_extensions/src/defuse/relayer.rscontract_extensions/src/defuse/signer.rscontract_extensions/src/defuse/state/fee.rscontract_extensions/src/defuse/state/garbage_collector.rscontract_extensions/src/defuse/state/mod.rscontract_extensions/src/defuse/state/salt.rscontract_extensions/src/defuse/tokens/mod.rscontract_extensions/src/defuse/tokens/nep141.rscontract_extensions/src/defuse/tokens/nep245.rscontract_extensions/src/escrow/mod.rscontract_extensions/src/lib.rscontract_extensions/src/poa/mod.rsdefuse/Cargo.tomldefuse/src/contract/accounts/account/entry/tests/legacy.rsdefuse/src/lib.rsdefuse/tests/accounts/auth_by_predecessor_id.rsdefuse/tests/accounts/force.rsdefuse/tests/accounts/manage_public_keys.rsdefuse/tests/accounts/mod.rsdefuse/tests/accounts/nonces.rsdefuse/tests/intents/ft_withdraw.rsdefuse/tests/intents/legacy_nonce.rsdefuse/tests/intents/mod.rsdefuse/tests/intents/native_withdraw.rsdefuse/tests/intents/public_key.rsdefuse/tests/intents/relayers.rsdefuse/tests/intents/simulate.rsdefuse/tests/intents/token_diff.rsdefuse/tests/intents/transfer.rsdefuse/tests/mod.rsdefuse/tests/state/fee.rsdefuse/tests/state/mod.rsdefuse/tests/state/salt.rsdefuse/tests/state/upgrade.rsdefuse/tests/storage/mod.rsdefuse/tests/tokens/mod.rsdefuse/tests/tokens/nep141.rsdefuse/tests/tokens/nep171.rsdefuse/tests/tokens/nep245/letter_gen.rsdefuse/tests/tokens/nep245/mod.rsdefuse/tests/tokens/nep245/mt_transfer_resolve_gas.rsescrow-swap/Cargo.tomlescrow-swap/tests/helpers/mod.rsescrow-swap/tests/partial_fills.rspoa-factory/Cargo.tomlpoa-factory/src/lib.rspoa-factory/tests/factory.rstest-utils/Cargo.tomltest-utils/src/fixtures.rstest-utils/src/lib.rstests/Cargo.tomltests/src/defuse_signer.rstests/src/env/builder.rstests/src/env/mod.rstests/src/env/state.rstests/src/env/storage.rstests/src/lib.rstests/src/nonce.rstests/src/tests/mod.rstests/src/tests/poa/mod.rstests/src/utils/mod.rstests/tests/defuse/intents.rstests/tests/defuse/mod.rstests/tests/defuse/tokens/mod.rstests/tests/defuse/tokens/nep141.rstests/tests/defuse/tokens/nep171.rstests/tests/defuse/tokens/nep245.rstests/tests/mod.rstests/tests/utils.rs
💤 Files with no reviewable changes (5)
- tests/src/utils/mod.rs
- poa-factory/src/lib.rs
- tests/src/tests/poa/mod.rs
- tests/src/tests/mod.rs
- defuse/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (22)
tests/src/env/state.rs (1)
tests/src/env/mod.rs (1)
generate_legacy_user_account_id(268-283)
tests/src/lib.rs (3)
escrow-swap/tests/helpers/mod.rs (1)
env(27-29)core/src/payload/mod.rs (1)
nonce(60-62)tests/src/env/mod.rs (1)
sandbox(250-252)
defuse/tests/tokens/nep171.rs (3)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)escrow-swap/tests/helpers/mod.rs (1)
env(27-29)tests/src/env/mod.rs (1)
sandbox(250-252)
tests/tests/mod.rs (1)
sandbox/src/helpers.rs (1)
read_wasm(6-18)
defuse/tests/tokens/nep245/mod.rs (2)
defuse/tests/tokens/nep245/letter_gen.rs (1)
new(12-14)contract_extensions/src/defuse/tokens/nep141.rs (2)
defuse_ft_withdraw(33-41)defuse_ft_withdraw(56-81)
tests/tests/defuse/mod.rs (2)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)tests/src/env/storage.rs (1)
tokens(230-233)
defuse/tests/intents/token_diff.rs (2)
tests/src/env/mod.rs (1)
sandbox(250-252)escrow-swap/tests/helpers/mod.rs (1)
env(27-29)
test-utils/src/fixtures.rs (1)
test-utils/src/random.rs (1)
random(82-84)
defuse/tests/intents/transfer.rs (1)
tests/src/tests/defuse/intents/transfer.rs (2)
transfer_intent(24-72)transfer_intent_to_defuse(77-199)
tests/tests/defuse/intents.rs (4)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)tests/src/env/storage.rs (1)
tokens(230-233)poa-factory/src/contract.rs (1)
token_id(197-203)tests/src/env/mod.rs (2)
create_user(136-147)create_token(103-111)
tests/src/env/storage.rs (3)
sandbox/src/account.rs (1)
signer(131-133)randomness/src/lib.rs (1)
make_true_rng(16-18)escrow-swap/tests/helpers/mod.rs (1)
env(27-29)
defuse/tests/intents/native_withdraw.rs (3)
escrow-swap/tests/helpers/mod.rs (1)
env(27-29)tests/src/env/mod.rs (1)
sandbox(250-252)test-utils/src/fixtures.rs (2)
ed25519_pk(16-18)secp256k1_pk(21-23)
defuse/tests/state/mod.rs (1)
defuse/tests/state/upgrade.rs (1)
upgrade(42-87)
defuse/tests/tokens/nep141.rs (6)
tests/src/env/storage.rs (1)
tokens(230-233)poa-factory/src/lib.rs (1)
tokens(35-35)escrow-swap/tests/helpers/mod.rs (1)
env(27-29)tests/src/env/mod.rs (1)
sandbox(250-252)near-utils/src/panic_on_clone.rs (1)
core(26-26)contract_extensions/src/defuse/intents.rs (1)
intents(53-53)
defuse/src/contract/accounts/account/entry/tests/legacy.rs (1)
test-utils/src/random.rs (2)
random(82-84)make_arbitrary(154-160)
defuse/tests/intents/legacy_nonce.rs (5)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)sandbox/src/account.rs (1)
signer(131-133)escrow-swap/tests/helpers/mod.rs (1)
env(27-29)tests/src/env/mod.rs (1)
sandbox(250-252)test-utils/src/random.rs (2)
random(82-84)make_arbitrary(154-160)
escrow-swap/tests/partial_fills.rs (1)
escrow-swap/tests/helpers/mod.rs (1)
env(27-29)
defuse/tests/intents/public_key.rs (4)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)escrow-swap/tests/helpers/mod.rs (1)
env(27-29)core/src/payload/mod.rs (1)
nonce(60-62)test-utils/src/fixtures.rs (1)
public_key(8-13)
tests/src/env/builder.rs (1)
escrow-swap/tests/helpers/mod.rs (1)
env(27-29)
defuse/tests/state/upgrade.rs (6)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)tests/src/env/storage.rs (1)
state(245-252)tests/src/env/mod.rs (1)
sandbox(250-252)sandbox/src/helpers.rs (1)
read_wasm(6-18)test-utils/src/fixtures.rs (3)
ed25519_pk(16-18)p256_pk(26-28)secp256k1_pk(21-23)escrow-swap/tests/helpers/mod.rs (1)
env(27-29)
tests/src/env/mod.rs (2)
escrow-swap/tests/helpers/mod.rs (1)
env(27-29)randomness/src/lib.rs (1)
make_true_rng(16-18)
contract_extensions/src/escrow/mod.rs (1)
sandbox/src/helpers.rs (1)
read_wasm(6-18)
⏰ 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 Reproducible
- GitHub Check: Build
- GitHub Check: Security Audit - deny
- 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 / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- 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 (80)
defuse/tests/accounts/nonces.rs (1)
19-33: LGTM! Clean import reorganization.The new import structure under
defuse_testsis well-organized with logical groupings:
- Contract extensions separated by functionality (
account_manager,intents,signer,state)- Clear separation between
env,nonce,sandbox, andutilsmodulesThis aligns well with the PR's goal of consolidating test utilities into a unified
defuse-testscrate.tests/tests/defuse/mod.rs (1)
1-2: Clean module structure for test organization.Both submodules are properly defined with their corresponding files present:
intents.rsandtokens/mod.rsexist in the expected locations.tests/src/env/storage.rs (2)
18-25: LGTM! Clean import consolidation.The reorganization of imports to use the new
defuse_contract_extensionscrate aligns well with the PR's objective of consolidating test utilities. The import structure is clear and well-organized.
32-35: LGTM! Env imports properly consolidated.The consolidation of
crate::envimports is clean and maintains good module organization.tests/tests/defuse/tokens/nep141.rs (1)
67-157: Solid test structure with comprehensive scenarios.The test properly:
- Sets up the environment with deployer as super admin
- Creates users and deploys receiver stub contract
- Handles storage deposits
- Constructs appropriate deposit messages based on the test case
- Verifies both FT and MT balances after the transfer
The use of
futures::join!for concurrent user/token creation is a nice optimization.tests/tests/defuse/tokens/nep171.rs (2)
172-207: Thorough ownership verification.The test properly verifies ownership in two independent ways:
- Direct NFT contract ownership check (lines 172-188)
- MT balance verification on the defuse contract (lines 190-207)
The descriptive assertion messages will help with debugging test failures.
68-73: Good documentation on naming constraint.The comment explaining why a named user is used (to avoid exceeding NEAR's 64-char account limit) is helpful for future maintainers.
tests/tests/defuse/tokens/mod.rs (1)
1-3: Module declarations properly reference existing implementation files.The module structure follows NEAR token standard naming conventions (NEP-141 for fungible tokens, NEP-171 for NFTs, NEP-245 for multi-token). The
nep245module file exists attests/tests/defuse/tokens/nep245.rsas expected.defuse/tests/state/mod.rs (1)
3-3: LGTM!The addition of the
upgrademodule follows the established pattern and integrates well with the existing test module structure.test-utils/Cargo.toml (1)
13-13: LGTM!The
defuse-cryptodependency with thearbitraryfeature correctly supports the cryptographic type usage in test fixtures, aligning with the import path changes intest-utils/src/fixtures.rs.tests/src/env/state.rs (1)
14-14: Import path simplified correctly.The updated import path reflects the reorganized module hierarchy, making the code cleaner and more maintainable.
test-utils/src/fixtures.rs (1)
2-6: LGTM!The import path update to use
defuse_crypto::PublicKeycorrectly aligns with the new dependency added intest-utils/Cargo.toml. The import reorganization improves code organization while maintaining all public function signatures.test-utils/src/lib.rs (1)
2-2: LGTM!The addition of the
fixturesmodule to the public API aligns well with the PR's goal of consolidating test utilities.defuse/tests/state/fee.rs (1)
1-11: LGTM!The import refactoring follows the same clean pattern as other test files, consolidating utilities under the
defuse_testsnamespace. The changes are consistent and maintain the same logical structure.defuse/tests/storage/mod.rs (1)
1-12: LGTM!The import consolidation is consistent with the broader refactoring effort. The grouped imports under
defuse_testsimprove code organization and maintainability.Cargo.toml (2)
80-84: Workspace dependencies added correctly.The new workspace dependencies
defuse-contract-extensionsanddefuse-testsare properly configured to support the refactored test infrastructure.
37-37: No circular dependencies detected in the workspace structure.The addition of
contract_extensionsto the workspace members is properly integrated. The tests crate depends ondefuse-contract-extensions, andcontract_extensionshas no reverse dependency on tests. All workspace dependencies are unidirectional with no circular references.defuse/tests/state/salt.rs (1)
1-10: Import consolidation is correct.All imported modules exist and are properly accessible through the
defuse_testsnamespace:
contract_extensions::defuse::state::{SaltManagerExt, SaltViewExt}fromcontract_extensions/src/defuse/state/sandbox::extensions::acl::AclExtfromsandbox/src/extensions/acl.rsutils::asserts::ResultAssertsExtfromtest-utils/src/asserts.rsenv::Envfromtests/src/env/The refactoring successfully consolidates test utilities under the
defuse_testsnamespace via re-exports intests/src/lib.rs, improving module organization consistently with the PR's objective.tests/tests/mod.rs (1)
1-8: LGTM! Clean test module organization.The module declarations and lazy-loaded WASM binary initialization follow good practices for test infrastructure.
defuse/tests/accounts/auth_by_predecessor_id.rs (1)
1-19: LGTM! Well-organized import consolidation.The import reorganization under the
defuse_testsnamespace improves test code maintainability by centralizing test utilities and extensions.defuse/tests/mod.rs (1)
3-7: LGTM! Clear test module structure.The submodule organization aligns well with the PR's objective of improving test separation.
tests/src/lib.rs (1)
3-9: LGTM! Appropriate public exposure for test utilities.Making these modules and re-exports public is correct for a shared test utilities crate (
defuse-tests), enabling other test crates to access the consolidated test infrastructure.defuse/tests/state/upgrade.rs (1)
15-35: LGTM! Consistent import consolidation.The reorganized imports under
defuse_testsmaintain consistency with the broader refactoring and improve the clarity of test dependencies.defuse/Cargo.toml (1)
69-83: LGTM!The dev-dependencies are correctly organized for the new test structure. The consolidation under
defuse-testsand addition ofdefuse-randomnessaligns well with the PR's objective to centralize test utilities.defuse/tests/tokens/nep245/mt_transfer_resolve_gas.rs (2)
1-17: LGTM!The import reorganization correctly consolidates test utilities under
defuse_tests. The module paths are well-structured and align with the new test infrastructure.
29-33: Consistent derive/format change.Removing
Displayand usingDebugformatting ({gen_mode:?}) is consistent. The log output will now showShortestPossibleorLongestPossiblewith debug formatting, which is acceptable for test diagnostics.Also applies to: 297-297
defuse/tests/accounts/manage_public_keys.rs (1)
3-16: LGTM!Import paths are correctly consolidated under
defuse_tests. The reorganization is clean withcontract_extensions::defuse::account_manager,env,sandbox, andutils::fixturesproperly separated. Re-exportingserde_json::jsonthroughnear_sdkis a nice cleanup.defuse/tests/accounts/force.rs (1)
1-25: LGTM!The import reorganization cleanly separates concerns:
defuse_tests::defuse_signerandnoncefor test utilitiesdefuse_tests::contract_extensions::defusefor contract interaction traitsdefuse_tests::sandbox::extensionsfor sandbox-specific utilitiesdefuse_tests::utilsfor assertions and fixturesThis structure improves maintainability by making test utility sources explicit.
tests/tests/defuse/tokens/nep245.rs (2)
11-22: LGTM!The import reorganization correctly uses
defuse_contract_extensionsfor contract extension traits anddefuse_testsfor test utilities. The separation between these two crates is appropriate.
633-635: Acceptable for complex integration test.The
clippy::too_many_linesallow is reasonable here. This integration test covers a complex multi-token transfer scenario with duplicate tokens, and splitting it would likely reduce clarity rather than improve it.defuse/tests/intents/relayers.rs (1)
1-13: LGTM!The import consolidation under
defuse_testsis well-organized. The nested structure clearly separates:
- Contract extensions (
intents,relayer)- Sandbox primitives (
SigningAccount,Signer,generate_secret_key)- Sandbox extensions (
acl)- Utility assertions
contract_extensions/src/defuse/tokens/nep141.rs (1)
6-6: LGTM!Importing
DepositMessagedirectly fromdefuse::tokensmakes the dependency on thedefusecrate explicit, which is clearer than a crate-relative path in this extension module.contract_extensions/src/defuse/deployer.rs (1)
6-6: LGTM! Import path correctly updated.The import path change from
crate::contract::config::DefuseConfigtodefuse::contract::config::DefuseConfigis correct given that this code has moved to thecontract_extensionscrate and needs to reference thedefusecrate explicitly.contract_extensions/src/defuse/mod.rs (1)
11-12: LGTM! Consistent alias pattern.The public re-export of
defuse as contractestablishes a consistent naming convention across thecontract_extensionsmodule, aligning with similar aliases in thepoaandescrowmodules.contract_extensions/src/poa/mod.rs (2)
3-3: LGTM! Consistent public alias pattern.The re-export of
defuse_poa_factory as contractfollows the same aliasing pattern established in thedefusemodule, providing a uniform API surface.
12-12: LGTM! Import path updated to use the new alias.The import path correctly references
defuse_poa_factory::contractusing the alias defined at line 3, maintaining consistency with the module reorganization.contract_extensions/src/defuse/intents.rs (1)
11-11: Import path is correct and properly exposesSimulationOutputfrom the defuse crate.The import
use defuse::simulation_output::SimulationOutput;correctly references the publicsimulation_outputmodule in the defuse crate, which publicly exportsSimulationOutput. This reflects the refactoring within defuse that moved the type from the intents module to its own dedicatedsimulation_outputmodule.defuse/tests/intents/legacy_nonce.rs (1)
7-15: LGTM! Clean consolidation of test utilities.The import reorganization successfully consolidates test utilities under the unified
defuse_testsnamespace, achieving the PR's objective of centralizing test infrastructure. The import tree is well-organized and all imported items are properly accessible and referenced in the test code.contract_extensions/Cargo.toml (1)
1-28: LGTM! Clean manifest structure.The manifest is well-organized with appropriate workspace references, clean feature flags that map to optional dependencies, and suitable crate types for contract extensions.
defuse/tests/tokens/nep171.rs (1)
5-15: LGTM! Import consolidation aligns with refactoring goals.The consolidated imports from
defuse_testsimprove organization and align with the test separation objectives.defuse/tests/intents/transfer.rs (2)
1-65: Well-structured transfer intent test.The test demonstrates proper intent creation, signing, execution, and balance verification flow. Good use of async/await and assertion patterns.
22-22: The hardcoded account ID is intentional and validates transfer behavior to non-existent accounts.The test explicitly verifies that the contract correctly handles transfers to accounts that don't undergo explicit creation (lines 58–63 confirm the balance reaches the non-existent account). This pattern is consistent across multiple test files and the passing assertions indicate the intended behavior works correctly. No changes needed.
defuse/tests/intents/mod.rs (2)
1-23: LGTM! Import reorganization improves test structure.The consolidation of imports under
defuse_testsand the addition ofserde_jsonsupport the test infrastructure refactoring objectives.
55-55: New transfer module properly integrated.The addition of
mod transfer;correctly exposes the new transfer intent test module.tests/Cargo.toml (2)
15-15: New contract extensions dependency properly integrated.The addition of
defuse-contract-extensionswith["poa", "defuse"]features aligns with the test separation refactoring objectives.
11-21: No action needed—feature change is intentional and correct.The near-sdk feature was changed from "unit-testing" to "arbitrary" as part of commit 6aa6204 ("feat: use cache from near-sdk and arbitrary from near-account-id"). The test crate uses
defuse-sandboxanddefuse-test-utilsfor its test environment and utilities, not near-sdk's "unit-testing" feature. The "arbitrary" feature is the correct choice because it enables theArbitrarytrait for near-sdk types (particularlyAccountIdvia near-account-id integration), which the test code relies on for generating test data.contract_extensions/src/lib.rs (1)
1-6: LGTM! Clean feature-gated module organization.The conditional module declarations follow standard Rust patterns and align well with the PR's objective of separating contract extensions into feature-gated modules.
defuse/tests/intents/ft_withdraw.rs (1)
7-19: LGTM! Clean consolidation of test imports.The import reorganization successfully consolidates test utilities under the
defuse_testscrate, improving code organization and aligning with the PR's objective to unify test infrastructure.escrow-swap/tests/partial_fills.rs (1)
1-29: LGTM! Import reorganization aligns with test infrastructure refactoring.The import path updates successfully integrate the new
defuse_contract_extensionsstructure and localhelpersmodule. The changes are consistent with the PR's objective to consolidate test utilities and improve organization.tests/src/nonce.rs (1)
19-28: The 15-byte nonce size is correct. The[u8; 15]size matches the pattern used incore/src/nonce/versioned.rs(line 64), where nonces forVersionedNonceare generated with the same size. TheExpirableNonce<T>type is generic, and different contexts use different sizes (15, 24, or 32 bytes) appropriately.Likely an incorrect or invalid review comment.
poa-factory/tests/factory.rs (1)
1-2: Import paths are correctly configured.Both
PoAFactoryDeployerExtandPoAFactoryExtare publicly defined incontract_extensions/src/poa/mod.rsand properly exported. TheRoletype is accessible viadefuse_poa_factory::contract. All imports are correctly wired in the workspace.contract_extensions/src/escrow/mod.rs (5)
1-1: LGTM! Clean public re-export.The public re-export of
defuse_escrow_swapprovides a unified surface for escrow-related components, aligning with the broader refactoring to consolidate extensions underdefuse_contract_extensions.
5-7: LGTM! Imports reorganized to use defuse_sandbox.The consolidated import from
defuse_sandboxis consistent with the PR's objective to unify test utilities and sandbox extensions.
13-25: LGTM! Trait-level lint attribute is more precise.Moving
#[allow(async_fn_in_trait)]from crate-level to trait-level provides better lint hygiene by limiting the scope of the allow directive.
57-60: LGTM! Consistent lint attribute application.The trait-level
#[allow(async_fn_in_trait)]attribute is consistently applied toEscrowExtView, matching the pattern used forEscrowExt.
10-11: Path resolution is correct across workspace members.The implementation in
sandbox/src/helpers.rscorrectly handlesCARGO_MANIFEST_DIRresolution. Theenv!("CARGO_MANIFEST_DIR")macro is evaluated at compile time for each crate, and the relative path navigation (../) correctly resolves to the repository rootres/directory regardless of whether called fromcontract_extensionsorescrow-swap. Thefs::canonicalize()call ensures absolute path validity with clear error messages on failure.defuse/tests/intents/token_diff.rs (1)
7-14: LGTM! Test imports successfully consolidated.The reorganized imports align with the PR's objective to consolidate test utilities under the
defuse_testscrate. The new import structure provides a clearer separation between:
- Contract extensions (
defuse_tests::contract_extensions::defuse::intents)- Sandbox extensions (
defuse_tests::sandbox::extensions::mt)- Test utilities (
defuse_tests::env,defuse_tests::defuse_signer)tests/src/defuse_signer.rs (2)
5-5: LGTM! Import path consolidated to defuse_contract_extensions.The updated import path aligns with the PR's objective to consolidate contract extensions under the
defuse_contract_extensionscrate, providing a unified surface for signer and state-related functionality.
13-53: LGTM! DefuseSignerExt provides useful test utilities.The trait extension provides convenient methods for generating unique nonces and signing defuse payloads in tests, with appropriate use of
TestRngfor test randomness.escrow-swap/tests/helpers/mod.rs (1)
24-24: LGTM! WASM source centralized to contract_extensions.The
ESCROW_SWAP_WASMis now sourced fromdefuse_contract_extensions::escrow, providing a single source of truth for the WASM artifact and aligning with the PR's consolidation objective.poa-factory/Cargo.toml (1)
38-43: No production code depends on the removedsandboxfeature—this change is safe.The removal of the
sandboxfeature flag and conversion ofdefuse-sandboxto a dev-dependency is safe. No production code (inpoa-factory/src) references the sandbox feature via cfg gates or otherwise. The sandbox dependency is appropriately used only in tests, where it now functions as a standard dev-dependency rather than a feature-gated one.defuse/tests/intents/native_withdraw.rs (1)
13-19: LGTM! Import consolidation aligns with test-separation refactor.The imports are correctly reorganized to use the centralized
defuse_testscrate, improving maintainability by consolidating test utilities under a single namespace.defuse/tests/tokens/nep141.rs (2)
3-11: LGTM! Clean import consolidation.The imports are well-organized under the
defuse_testsnamespace, properly grouping contract extensions, signer utilities, environment, and sandbox extensions.
244-316: Good test coverage for force withdraw authorization.The
ft_force_withdrawtest properly validates the authorization flow: first ensuring the operation fails without theUnrestrictedWithdrawerrole, then succeeding after the role is granted. This is a good pattern for testing role-based access control.tests/src/env/builder.rs (2)
188-203: LGTM! POA factory deployment with appropriate test permissions.The function correctly sets up the POA factory with root as super admin and grants both
TokenDeployerandTokenDepositerroles for test environment flexibility.
10-13: The import pathdefuse_contract_extensions::poa::contract::contract::Roleis correct. The firstcontractrefers to the re-exporteddefuse_poa_factorycrate (viapub use defuse_poa_factory as contractinpoa/mod.rs), and the secondcontractrefers to thecontract.rsmodule within that crate where theRoleenum is defined. This nested structure is intentional and not a typo.Likely an incorrect or invalid review comment.
escrow-swap/Cargo.toml (1)
36-43: LGTM! Dev-dependencies properly configured for testing.The dev-dependencies are well-structured with appropriate features enabled for the test infrastructure.
defuse/tests/intents/simulate.rs (3)
1-1: LGTM! Crate-local import for intent event types.The import correctly references types from the local
crate::intentsmodule for event handling.
23-42: LGTM! Well-organized imports from defuse_tests.The imports are properly structured, grouping:
- Contract extensions (account_manager, deployer, intents)
- Signer utilities
- Environment and nonce extraction
- Sandbox API types and extensions
- Test fixtures
This aligns well with the test-separation refactoring goals.
911-934: Good test for nonce replay protection.The test properly verifies that simulating an intent with an already-used nonce fails. This is important for ensuring intent replay attacks are prevented.
tests/tests/defuse/intents.rs (2)
21-27: LGTM! Clean test expectation struct.The
TransferCallExpectationstruct nicely encapsulates the test parameters for different MT receiver modes, making the parameterized tests readable.
156-187: Comprehensive test coverage for MT receiver scenarios.The parameterized test cases effectively cover edge cases:
AcceptAll: Normal acceptanceReturnValue(300): Partial refundReturnValue(2000): Malicious over-refund (clamped to original amount)Panic: Receiver failure with full refundLargeReturn: Malicious large return with full refundThis ensures robust handling of various receiver behaviors.
defuse/tests/tokens/nep245/mod.rs (4)
11-18: LGTM! Proper imports for NEP245 multi-token tests.The imports correctly pull in the required extensions for deploying Defuse instances and performing MT operations.
21-287: Comprehensive multi-token enumeration test.The test thoroughly validates token enumeration across deposit/withdraw lifecycle, properly handling:
- Initial state verification
- Token appearance after deposits
- Multi-user token ownership tracking
- Token disappearance after full withdrawal
- Return to initial state verification
Good use of
from_token_indexto account for pre-existing persistent state tokens.
379-389: Good range boundary testing.The nested loop exhaustively tests all valid range combinations for
mt_tokens, ensuring the range-based enumeration API works correctly across different start/end indices.
827-873: LGTM! Final balance verification after multiple withdrawals.The test correctly verifies the cumulative effect of the three withdrawal operations:
- ft1: 100 - 10 - 10 - 10 = 70
- ft2: 200 - 0 - 20 - 20 = 160
- ft3: 300 - 0 - 0 - 30 = 270
This ensures the withdrawal accounting is accurate across multiple operations.
defuse/tests/intents/public_key.rs (1)
11-15: LGTM!The import reorganization correctly consolidates test utilities under the
defuse_testsnamespace, aligning with the PR's test separation refactoring. The grouped imports improve readability and reflect the new module structure.tests/src/env/mod.rs (3)
7-24: LGTM!The import reorganization properly reflects the new module structure introduced by the
contract_extensionsanddefuse_testscrates. The consolidated imports fromdefuse_contract_extensionsfor account management, token operations, and PoA factory extensions align well with the PR's test separation objectives.
254-256: LGTM!Returning a mutable reference to a struct field is valid in
const fncontext. This change appropriately tightens the API contract.
64-66: No action required.Sandbox::root()is properly declared as aconst fn(sandbox/src/lib.rs:73), so the delegation in this method is valid and will compile correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @contract_extensions/src/defuse/nonce.rs:
- Around line 32-35: The call to defuse_contract.current_salt().await uses
expect and panics; replace that with the `?` operator to propagate errors (e.g.,
let salt = defuse_contract.current_salt().await?;) so the enclosing function
(which returns anyhow::Result<Nonce>) returns the error instead of aborting;
ensure any necessary trait bounds/imports for ? are present and no expect
remains for current_salt().
🧹 Nitpick comments (7)
contract_extensions/Cargo.toml (1)
14-25: Consider making test utilities optional or dev-dependencies.
defuse-sandbox,defuse-test-utils, anddefuse-randomnessappear to be test/sandbox utilities but are declared as regular non-optional dependencies. This means any crate depending ondefuse-contract-extensionswill transitively pull in these test utilities even in production builds.Consider either:
- Moving them to
[dev-dependencies]if only used in tests- Making them optional behind a
sandboxortestingfeature flag🔎 Proposed refactor to gate test utilities behind a feature
[dependencies] -defuse-sandbox = {workspace = true} +defuse-sandbox = {workspace = true, optional = true} defuse = {workspace = true, features = ["contract"], optional = true} defuse-poa-factory = {workspace = true, features = ["contract"], optional = true} defuse-escrow-swap = {workspace = true, features = ["default"], optional = true} defuse-core = { workspace = true } defuse-serde-utils = { workspace = true } defuse-nep245 = { workspace = true } near-contract-standards.workspace = true near-sdk = { workspace = true } -defuse-test-utils.workspace = true -defuse-randomness.workspace = true +defuse-test-utils = {workspace = true, optional = true} +defuse-randomness = {workspace = true, optional = true} [features] +default = ["sandbox"] +sandbox = ["dep:defuse-sandbox", "dep:defuse-test-utils", "dep:defuse-randomness"] poa = ["dep:defuse-poa-factory"] defuse = ["dep:defuse"] escrow = ["dep:defuse-escrow-swap"]contract_extensions/src/defuse/mod.rs (1)
13-13: Consider documenting the purpose of the contract alias.The
pub use defuse as contract;re-export creates a convenience alias, but its purpose may not be immediately clear to users of this module. Consider adding a doc comment explaining when to usecontractvs. directly importing fromdefuse.tests/tests/defuse/tokens/nep245.rs (1)
74-74: Remove redundant imports.The types
AmountsandTransferare already imported at the top of the file (lines 2, 4). This local import inside the test function is unnecessary.🔎 Proposed fix
- use defuse::core::{amounts::Amounts, intents::tokens::Transfer}; - let env = Env::builder().deployer_as_super_admin().build().await;tests/tests/defuse/intents.rs (2)
22-28: Consider whether TransferCallExpectation needs to be public.The
TransferCallExpectationstruct is marked aspub, but it appears to only be used within test cases in this file. Unless it's intended for reuse across multiple test modules, it could have module-level visibility instead.
191-193: Move local imports to the top of the file for consistency.These imports inside the test function could be moved to the file-level imports for better organization and consistency with Rust conventions.
🔎 Proposed fix
Move these imports to the top of the file alongside the other imports, and remove them from inside the function:
use defuse_contract_extensions::defuse::deployer::DefuseExt; use defuse_contract_extensions::defuse::intents::ExecuteIntentsExt; use defuse_contract_extensions::defuse::signer::DefaultDefuseSignerExt; use defuse_sandbox::extensions::ft::FtViewExt; use defuse_sandbox::extensions::mt::MtViewExt; +use defuse_sandbox::tx::FnCallBuilder; use multi_token_receiver_stub::MTReceiverMode; -use near_sdk::{AccountId, Gas, serde_json}; +use near_sdk::{AccountId, Gas, NearToken, serde_json}; use rstest::rstest;Then remove lines 191-193 from inside the function.
contract_extensions/src/defuse/signer.rs (1)
53-76: Consider extracting the 120-second timeout to a constant.The 120-second deadline appears in both this file (line 63) and
nonce.rs(line 30). Extracting to a shared constant would prevent drift.🔎 Suggested approach
// In a shared location (e.g., mod.rs or constants module) pub const DEFAULT_DEADLINE_SECS: u64 = 120;Then use it in both places:
-let deadline = Deadline::timeout(std::time::Duration::from_secs(120)); +let deadline = Deadline::timeout(std::time::Duration::from_secs(DEFAULT_DEADLINE_SECS));contract_extensions/src/defuse/nonce.rs (1)
23-56: Consider making unused-selfmethods into associated or free functions.Both
generate_unique_nonceandcreate_random_salted_noncedon't useself, yet they're defined as instance methods with a blanket impl for all types. This is an unusual pattern.If method syntax on a specific type (like
SigningAccount) isn't required, these could be standalone functions or associated functions withoutself:🔎 Alternative design
// As standalone functions in a module: pub async fn generate_unique_nonce( defuse_contract: &Account, deadline: Option<Deadline>, ) -> anyhow::Result<Nonce> { // ... implementation } pub fn create_random_salted_nonce(salt: Salt, deadline: Deadline, rng: impl Rng) -> Nonce { // ... implementation }This would be more idiomatic when
selfisn't used. However, if the method-call syntax is preferred for ergonomics in test code, the current approach is acceptable.
📜 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 (11)
contract_extensions/Cargo.tomlcontract_extensions/src/defuse/mod.rscontract_extensions/src/defuse/nonce.rscontract_extensions/src/defuse/signer.rstests/src/env/mod.rstests/src/env/storage.rstests/src/lib.rstests/tests/defuse/intents.rstests/tests/defuse/tokens/nep141.rstests/tests/defuse/tokens/nep171.rstests/tests/defuse/tokens/nep245.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/tests/defuse/tokens/nep141.rs
- tests/src/env/storage.rs
🧰 Additional context used
🧬 Code graph analysis (8)
tests/tests/defuse/tokens/nep171.rs (2)
tests/src/env/mod.rs (1)
builder(57-59)contract_extensions/src/defuse/signer.rs (1)
sign_defuse_payload_default(55-74)
tests/src/lib.rs (2)
escrow-swap/tests/helpers/mod.rs (1)
env(26-28)tests/src/env/mod.rs (1)
sandbox(251-253)
tests/tests/defuse/tokens/nep245.rs (5)
sandbox/src/account.rs (1)
signer(131-133)escrow-swap/tests/helpers/mod.rs (1)
env(26-28)near-utils/src/panic_on_clone.rs (1)
core(26-26)contract_extensions/src/defuse/intents.rs (1)
intents(53-53)tests/src/env/storage.rs (1)
tokens(230-233)
tests/tests/defuse/intents.rs (3)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)escrow-swap/tests/helpers/mod.rs (1)
env(26-28)tests/src/env/mod.rs (2)
create_user(137-148)create_token(104-112)
contract_extensions/src/defuse/signer.rs (3)
contract_extensions/src/defuse/intents.rs (1)
intents(53-53)core/src/payload/mod.rs (1)
nonce(60-62)deadline/src/lib.rs (1)
timeout(44-46)
contract_extensions/src/defuse/nonce.rs (3)
test-utils/src/random.rs (1)
random(82-84)core/src/payload/mod.rs (1)
nonce(60-62)deadline/src/lib.rs (1)
timeout(44-46)
contract_extensions/src/defuse/mod.rs (1)
core/src/payload/mod.rs (1)
nonce(60-62)
tests/src/env/mod.rs (2)
escrow-swap/tests/helpers/mod.rs (1)
env(26-28)randomness/src/lib.rs (1)
make_true_rng(16-18)
🔇 Additional comments (18)
contract_extensions/Cargo.toml (2)
27-30: LGTM on feature definitions.The feature flags correctly use the
dep:syntax to enable optional dependencies, which is the modern Cargo convention for avoiding implicit feature names.
8-9: Thecdylibcrate type is consistent with the workspace pattern and is intentional.All contract-related crates in the repository (poa-factory, escrow-swap, defuse, poa-token, and others) use
crate-type = ["cdylib", "rlib"]. This is a standard workspace convention and should remain unchanged.Likely an incorrect or invalid review comment.
tests/src/env/mod.rs (5)
7-26: LGTM! Import reorganization aligns with the refactoring objectives.The import consolidation successfully moves contract extensions to the
defuse_contract_extensionsnamespace and introduces the necessary trait imports (GenerateNonceExt,PoAFactoryExt) for the updated method calls later in the file.
255-257: LGTM! Simple const accessor for sandbox.The
const fnconversion forsandbox_mutis straightforward since it only returns a mutable reference to a field.
96-102: Thepoa_factory_deploy_tokenmethod exists inPoAFactoryExtwith the correct signature and the method call passes valid arguments. No issues found.
69-72: Actual trait method signature differs from expected.The
GenerateNonceExt::generate_unique_noncetrait method signature is:async fn generate_unique_nonce( &self, defuse_contract: &Account, deadline: Option<Deadline>, ) -> anyhow::Result<Nonce>The second parameter is
defuse_contract: &Account, notaccount: &AccountIdRefas stated in the verification request. The code at lines 69-72 correctly passes&self.defuse(anAccountreference), which matches the actual trait signature.Likely an incorrect or invalid review comment.
65-67: No action required —Sandbox::root()is already a const function.The const fn constraint is satisfied.
Sandbox::root()insandbox/src/lib.rs(line 73) is correctly defined aspub const fn root(&self) -> &SigningAccount, returning a reference without any blocking operations. The change at line 65 compiles correctly.contract_extensions/src/defuse/mod.rs (1)
7-7: LGTM! New nonce module added.The addition of the nonce module aligns with the PR's goal of consolidating contract extensions.
tests/src/lib.rs (1)
3-7: LGTM! Test utilities consolidated effectively.The new
envmodule and re-exports provide a clean, unified interface for test utilities, aligning well with the PR's objective of test separation and consolidation.tests/tests/defuse/tokens/nep245.rs (2)
11-23: LGTM! Imports successfully migrated to new workspace structure.The updated imports correctly reference the new
defuse_contract_extensionsanddefuse_testsnamespaces, supporting the PR's test separation objectives.
634-808: Test function is very long but adequately handles complex scenario.The test
mt_transfer_call_duplicate_tokens_with_stub_execute_and_refundis marked with#[allow(clippy::too_many_lines)], which is appropriate given the complexity of testing duplicate token transfers with stub execution and refund logic. The test thoroughly validates the expected behavior across multiple token types and refund scenarios.tests/tests/defuse/tokens/nep171.rs (1)
1-209: LGTM! Well-structured NEP171 NFT transfer tests.This new test file provides comprehensive coverage of NFT transfer scenarios using parameterized tests. The test cases cover success paths (nothing_to_refund), partial refunds, error conditions (receiver_panics), and security scenarios (malicious_receiver). The use of
rstestfor parameterization and the clear expectation struct make the tests maintainable and easy to understand.tests/tests/defuse/intents.rs (2)
1-21: LGTM! Imports successfully updated for new workspace structure.The imports have been correctly updated to reference
defuse_contract_extensionsanddefuse_tests, supporting the PR's objective of consolidating test utilities.
202-210: LGTM! Inline deployment simplifies test setup.The refactoring to use
env.deploy_sub_contractdirectly within the test improves readability and makes the test setup more explicit.contract_extensions/src/defuse/signer.rs (2)
1-22: LGTM!The trait design is clean. The
DefuseSignerExttrait appropriately abstracts the signing behavior with a generic message type bounded bySerialize.
24-51: LGTM!The implementation correctly constructs the
Nep413Payloadand delegates signing. The.unwrap()calls are acceptable in this sandbox/test utility context.contract_extensions/src/defuse/nonce.rs (2)
1-9: LGTM!The imports are appropriate for the sandbox/test extension context.
11-21: LGTM!The trait design is clean. The clone on line 18 may be necessary if
extract_defuse_payloadconsumes self.
| let salt = defuse_contract | ||
| .current_salt() | ||
| .await | ||
| .expect("should be able to fetch salt"); |
There was a problem hiding this comment.
Use ? instead of expect for consistent error handling.
The function returns anyhow::Result<Nonce>, so the error should be propagated with ? rather than panicking with expect.
🔎 Proposed fix
let salt = defuse_contract
.current_salt()
.await
- .expect("should be able to fetch salt");
+ .map_err(|e| anyhow::anyhow!("failed to fetch salt: {e}"))?;Or if current_salt() already returns an anyhow::Result:
- .expect("should be able to fetch salt");
+?;📝 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 salt = defuse_contract | |
| .current_salt() | |
| .await | |
| .expect("should be able to fetch salt"); | |
| let salt = defuse_contract | |
| .current_salt() | |
| .await | |
| .map_err(|e| anyhow::anyhow!("failed to fetch salt: {e}"))?; |
🤖 Prompt for AI Agents
In @contract_extensions/src/defuse/nonce.rs around lines 32-35, The call to
defuse_contract.current_salt().await uses expect and panics; replace that with
the `?` operator to propagate errors (e.g., let salt =
defuse_contract.current_salt().await?;) so the enclosing function (which returns
anyhow::Result<Nonce>) returns the error instead of aborting; ensure any
necessary trait bounds/imports for ? are present and no expect remains for
current_salt().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Cargo.toml (1)
37-37: Consider maintaining alphabetical order in workspace members.The workspace members list is not fully alphabetized. "contract_extensions" appears after "wnear", while "tests" appears earlier between "serde-utils" and "test-utils". Consider moving "contract_extensions" to maintain alphabetical ordering for better maintainability.
📝 Suggested ordering
"bitmap", "borsh-utils", + "contract_extensions", "controller",And keep "tests" in its current position.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Cargo.tomlrust-toolchain
⏰ 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). (8)
- GitHub Check: Check
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- 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 security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- 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-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Security Audit - report
🔇 Additional comments (3)
Cargo.toml (2)
45-45: LGTM! Rust version is consistent with rust-toolchain.The rust-version correctly matches the toolchain channel specified in the rust-toolchain file.
80-80: LGTM! Path dependencies are correctly specified.The new workspace dependencies for
defuse-contract-extensionsanddefuse-testsare properly configured with correct paths.Also applies to: 84-84
rust-toolchain (1)
2-2: Rust 1.87.0 is a valid stable release (released May 15, 2025).Be aware that this version removes support for the i586-pc-windows-msvc target. If the project targets this platform, this is a breaking change to address.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @Cargo.toml:
- Line 80: Update the path value for the defuse-contract-extensions workspace
member in Cargo.toml to match the renamed directory: change the current
defuse-contract-extensions.path entry that references "contract_extensions" so
it points to the kebab-case directory name "contract-extensions" (i.e., update
the string value for defuse-contract-extensions.path).
- Line 37: The workspace member currently declared as "contract_extensions" uses
snake_case and is out of alphabetical order; rename the directory to kebab-case
("contract-extensions"), update the member declaration in Cargo.toml from
"contract_extensions" to "contract-extensions", and move that entry into its
correct alphabetical position among the other members (so it follows the
ordering pattern established by entries like "contract-api" and "contracts" as
appropriate). Ensure the filesystem directory is also renamed to
contract-extensions to match the Cargo.toml change.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
74-74: [email protected] is valid; consider evaluating the newer 0.18.0 release.Version 0.17.0 is published and not yanked. However, version 0.18.0 was released on January 1, 2026, just 11 days ago. Evaluate whether to upgrade to 0.18.0 depending on the changes it includes (breaking changes, bug fixes, etc.). The current pinning to 0.17.0 is sound for reproducibility; the upgrade is optional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci.ymlCargo.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). (10)
- GitHub Check: Security Audit - deny
- GitHub Check: Build
- GitHub Check: Build Reproducible
- 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 / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- 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 (1)
Cargo.toml (1)
84-84: LGTM!The
defuse-testsworkspace dependency correctly corresponds to the existing"tests"workspace member declared on line 31.
| "token-id", | ||
| "ton-connect", | ||
| "wnear", | ||
| "contract_extensions", |
There was a problem hiding this comment.
Use kebab-case for directory name to maintain consistency.
The new workspace member "contract_extensions" uses snake_case, which is inconsistent with all other workspace members that use kebab-case (e.g., "admin-utils", "auth-call", "escrow-swap").
Additionally, the member is placed at the end of the list, breaking the alphabetical ordering maintained in lines 4-36.
📝 Proposed fix
- Rename the directory from
contract_extensionstocontract-extensions - Update the member declaration and move it to its alphabetical position:
"borsh-utils",
+ "contract-extensions",
"controller",
"core",And remove from the end:
"wnear",
- "contract_extensions",
]Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @Cargo.toml at line 37, The workspace member currently declared as
"contract_extensions" uses snake_case and is out of alphabetical order; rename
the directory to kebab-case ("contract-extensions"), update the member
declaration in Cargo.toml from "contract_extensions" to "contract-extensions",
and move that entry into its correct alphabetical position among the other
members (so it follows the ordering pattern established by entries like
"contract-api" and "contracts" as appropriate). Ensure the filesystem directory
is also renamed to contract-extensions to match the Cargo.toml change.
| "text", | ||
| ] } | ||
| defuse-wnear.path = "wnear" | ||
| defuse-contract-extensions.path = "contract_extensions" |
There was a problem hiding this comment.
Update path to match the corrected directory name.
Once the directory is renamed to use kebab-case (see comment on line 37), update this path accordingly.
📝 Proposed fix
-defuse-contract-extensions.path = "contract_extensions"
+defuse-contract-extensions.path = "contract-extensions"📝 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.
| defuse-contract-extensions.path = "contract_extensions" | |
| defuse-contract-extensions.path = "contract-extensions" |
🤖 Prompt for AI Agents
In @Cargo.toml at line 80, Update the path value for the
defuse-contract-extensions workspace member in Cargo.toml to match the renamed
directory: change the current defuse-contract-extensions.path entry that
references "contract_extensions" so it points to the kebab-case directory name
"contract-extensions" (i.e., update the string value for
defuse-contract-extensions.path).
| poa = ["dep:defuse-poa-factory"] | ||
| defuse = ["dep:defuse"] | ||
| escrow = ["dep:defuse-escrow-swap"] No newline at end of file |
There was a problem hiding this comment.
i would enable all features by default
|
|
||
| #[rstest] | ||
| #[tokio::test] | ||
| async fn partial_fills(#[future(awt)] env: Env) { |
There was a problem hiding this comment.
i think concsensus was to move all the e2e tests to defuse-tests crate?
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.