WIP: Feat/global token contracts #162
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds versioned on‑chain state and explicit no‑registration control to poa‑token, exposes per‑token full‑access key APIs and optional global‑contract deployment in poa‑factory, updates build tasks/features and workspace deps, and simplifies tests by removing parameterized no_registration branches. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PoAFactory as PoA Factory
participant TokenAccount
participant GlobalReg as Global Contract
Caller->>PoAFactory: deploy_token(token, metadata, no_registration)
activate PoAFactory
PoAFactory->>TokenAccount: create account & transfer init balance
alt global_contracts enabled
PoAFactory->>GlobalReg: use_global_contract_by_account_id(account_id)
GlobalReg-->>TokenAccount: token logic referenced (no WASM deploy)
else
PoAFactory->>TokenAccount: deploy POA_TOKEN_WASM to account
end
PoAFactory->>TokenAccount: call initialization (metadata, no_registration)
PoAFactory-->>Caller: return Promise
deactivate PoAFactory
sequenceDiagram
participant User
participant Token as poa-token Contract
participant StateLayer as Versioned State Layer
User->>Token: storage_deposit(account_id, ...)
activate Token
alt no_registration == true
Token->>Token: require caller == owner or current_account
end
Token->>StateLayer: update balances / registrations
StateLayer-->>Token: persisted
Token-->>User: success
deactivate Token
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 2
🧹 Nitpick comments (3)
tests/src/tests/defuse/env/builder.rs (1)
33-33: Consider renamingEnv.disable_registrationfor consistency.The builder field has been renamed from
disable_registrationtono_registration, but Line 137 maps it back todisable_registrationwhen constructingEnv. For consistency with theNoRegistrationtrait naming, consider also renaming theEnvstruct field tono_registration.Apply this pattern to the
Envstruct definition (in the file definingEnv):pub struct Env { // ... other fields - pub disable_registration: bool, + pub no_registration: bool, }And update Line 137:
- disable_registration: self.no_registration, + no_registration: self.no_registration,Also applies to: 80-83, 137-137
Makefile.toml (1)
100-107: Consider adding global contracts variant for reproducible builds.The
build-reproducibletask usesbuild-poa-factory-reproduciblewhich doesn't enable theglobal_contractsfeature, unlike the non-reproducible default build. If production deployments require global contracts, the reproducible build should also enable this feature.poa-token/src/contract/mod.rs (1)
120-120: Consider removing or making this log conditional.The hardcoded log message "GLOBAL SMC LOG: ft_deposit called on global contract" will fire on every
ft_depositcall. If this contract is used in both global and non-global scenarios, this message could be misleading. Consider:
- Removing the log if it's for temporary debugging, or
- Making it conditional based on deployment mode, or
- Using a more generic message if all deployments are global contracts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.toml(1 hunks)Makefile.toml(6 hunks)poa-factory/Cargo.toml(2 hunks)poa-factory/src/contract.rs(5 hunks)poa-factory/src/lib.rs(1 hunks)poa-token/Cargo.toml(1 hunks)poa-token/src/contract/mod.rs(8 hunks)poa-token/src/contract/state.rs(1 hunks)poa-token/src/lib.rs(2 hunks)tests/src/tests/defuse/env/builder.rs(3 hunks)tests/src/tests/defuse/env/mod.rs(2 hunks)tests/src/tests/defuse/env/storage.rs(1 hunks)tests/src/tests/defuse/intents/ft_withdraw.rs(7 hunks)tests/src/tests/defuse/intents/legacy_nonce.rs(1 hunks)tests/src/tests/defuse/intents/mod.rs(3 hunks)tests/src/tests/defuse/intents/public_key.rs(2 hunks)tests/src/tests/defuse/intents/relayers.rs(1 hunks)tests/src/tests/defuse/intents/simulate.rs(6 hunks)tests/src/tests/defuse/intents/token_diff.rs(4 hunks)tests/src/tests/defuse/storage/mod.rs(2 hunks)tests/src/tests/defuse/tokens/nep141/mod.rs(6 hunks)tests/src/tests/defuse/tokens/nep245/mod.rs(6 hunks)tests/src/tests/poa/factory.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
tests/src/tests/defuse/intents/legacy_nonce.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/intents/public_key.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
poa-token/src/lib.rs (1)
tests/src/tests/defuse/env/builder.rs (1)
no_registration(80-83)
tests/src/tests/defuse/intents/simulate.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/storage/mod.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/intents/mod.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/intents/ft_withdraw.rs (3)
tests/src/tests/defuse/env/mod.rs (2)
builder(58-60)new(62-64)tests/src/utils/mt.rs (3)
mt_contract_balance_of(53-58)mt_contract_balance_of(211-226)mt_contract_balance_of(425-434)tests/src/tests/defuse/intents/mod.rs (1)
new(34-41)
tests/src/tests/defuse/tokens/nep141/mod.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/tokens/nep245/mod.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/intents/relayers.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/poa/factory.rs (1)
tests/src/tests/defuse/env/builder.rs (1)
no_registration(80-83)
poa-factory/src/contract.rs (1)
poa-factory/src/lib.rs (3)
deploy_token(25-30)add_token_full_access_key(14-14)delete_token_full_access_key(18-18)
poa-token/src/contract/mod.rs (2)
tests/src/tests/defuse/env/storage.rs (1)
state(243-250)tests/src/tests/defuse/env/builder.rs (1)
no_registration(80-83)
⏰ 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-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- 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-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: 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 (44)
tests/src/tests/defuse/intents/public_key.rs (1)
20-20: LGTM! Environment setup simplified.The removal of
.no_registration(true)aligns with the PR's goal to standardize environment initialization across tests.Also applies to: 63-63
tests/src/tests/defuse/intents/ft_withdraw.rs (3)
21-21: LGTM! Test parameterization removed.The function signatures have been simplified by removing the
no_registrationparameter, aligning with the PR's goal to standardize test flows.Also applies to: 217-217
27-27: LGTM! Environment setup simplified.The environment now uses default initialization without the
no_registrationflag.Also applies to: 220-220
112-125: LGTM! Added wNEAR preparation steps.The test now explicitly wraps NEAR and deposits wNEAR to prepare for storage deposit operations, making the test flow more explicit and deterministic.
poa-factory/src/contract.rs (3)
249-268: LGTM! Token full access key management implemented correctly.The
TokenFullAccessKeystrait implementation correctly delegates access key operations to individual token contracts using the appropriate external contract calls.
24-30: The balance reduction withglobal_contractsis intentional and correct—no issue exists.When
global_contractsis enabled (line 136-140), the token contract doesn't deploy its own WASM bytecode. Instead, it references a pre-deployed shared global contract via.use_global_contract_by_account_id(). Consequently, 5 milliNEAR is appropriate to cover only account creation and state storage (~500B as noted in the comment). Without global contracts (line 131-134), the full WASM is deployed per token account, requiring the higher 3 NEAR balance. The deposit validation at lines 119-122 correctly accounts for actual storage costs in both cases.
104-153: Based on my verification of the codebase, I can now provide the rewritten review comment:
The current implementation correctly handles both concerns raised.
The
no_registration: Option<bool>parameter is intentional and idiomatic. It wraps a plainboolwith a sensible default (DEFAULT_NO_REGISTRATION = true), allowing callers to either explicitly pass a value or rely on the default. This is the standard Rust pattern for optional initialization parameters, as demonstrated by the poa-token contract'snew()function which acceptsOption<bool>and unwraps it with a default.Regarding the TODO comment about reverting on failure with
use_global_contract_by_account_id: NEAR SDK's Promise model provides implicit transactional guarantees. The entire Promise chain (account creation, transfer, contract deployment, and function call) is atomic. Ifuse_global_contract_by_account_idfails, the entire transaction reverts, making explicit revert handling unnecessary. The TODO is a valid design question but reflects the inherent guarantees of the NEAR protocol rather than a missing safety mechanism.tests/src/tests/defuse/intents/legacy_nonce.rs (1)
18-18: LGTM! Environment setup simplified.The removal of
.no_registration(true)aligns with the PR's standardization of test environment initialization.tests/src/tests/defuse/storage/mod.rs (1)
32-32: LGTM! More explicit test configuration.Replacing
.no_registration(false)with.disable_ft_storage_deposit()makes the test setup more explicit about disabling automatic storage deposits, which aligns better with the intent of these storage deposit tests.Also applies to: 124-124
poa-factory/Cargo.toml (1)
24-24: LGTM! Global contracts feature added.The new
global_contractsfeature is properly configured and wired to thenear-sdk/global-contractsdependency feature. The build command correctly enables bothcontractandglobal_contractsfeatures.Also applies to: 39-40
tests/src/tests/defuse/env/storage.rs (1)
106-106: LGTM! Updated to match new factory API.The addition of the
trueargument forno_registrationcorrectly aligns with the expandedpoa_factory_deploy_tokensignature that now accepts ano_registrationparameter.Cargo.toml (1)
94-94: No breaking changes between near-workspaces 0.20 and 0.21; upgrade is safe.Based on verification of the near-workspaces CHANGELOG, version 0.21.0 (released 2025-08-15) contains no breaking changes. The only change is an internal dependency update from near-* 0.30 to 0.31. Your near-sdk 5.15 is compatible with near-* 0.31, and the extensive codebase usage of near-workspaces types and APIs (Account, Contract, Worker, call methods, etc.) requires no modifications.
poa-token/src/lib.rs (2)
15-22: LGTM! Well-designed trait addition.The
NoRegistrationtrait cleanly extends the token contract API with runtime configuration for registration behavior. The security requirement for 1 yⓃ attachment onset_no_registrationis appropriately documented.
36-36: Public API extension looks good.Adding
NoRegistrationto thePoaFungibleTokentrait bounds appropriately extends the contract interface.poa-token/Cargo.toml (2)
14-27: Build simplification aligns with runtime configuration approach.Removing the separate
no_registrationreproducible build variant is appropriate since registration behavior is now configurable at runtime via theNoRegistrationtrait.
32-43: New dependencies support versioned state management.The added dependencies (
defuse-borsh-utils,impl-tools,rstest,defuse-test-utils,arbitrary_with) align with the PR's introduction of versioned state management and enhanced testing capabilities.tests/src/tests/defuse/tokens/nep141/mod.rs (2)
35-36: Test simplification may affect NEP-141 coverage.These NEP-141 token tests have been simplified to use default environment configuration. Verify that FT deposit, withdrawal, and transfer behaviors don't vary based on registration mode, especially around storage deposit handling.
Also applies to: 75-76, 108-111, 190-193
257-262: Appropriate use ofdeployer_as_super_adminfor privileged operations.These tests correctly retain
deployer_as_super_admin()since they need to grant roles (Role::UnrestrictedWithdrawer) which requires admin permissions.Also applies to: 373-380
tests/src/tests/defuse/env/mod.rs (1)
103-109: Deployment simplification looks good.Consolidating token deployment into a single path with a boolean
no_registrationparameter improves maintainability and aligns with the runtime configuration approach.tests/src/tests/defuse/intents/mod.rs (1)
217-220: Test simplification is consistent with PR objectives.These intent tests have been appropriately simplified to use default environment configuration, aligning with the broader PR changes that move
no_registrationfrom a test-time parameter to a runtime configuration option.Also applies to: 308-312, 367-368
tests/src/tests/defuse/intents/relayers.rs (1)
14-18: Test coverage reduction concern is unfounded—relayer key management does not depend on registration mode.The
no_registrationflag affects token registration in POA token contracts and is completely independent of relayer key operations. The relayer key management functions (add_relayer_keyanddelete_relayer_key) have no logic or conditional behavior based on registration state—they only manage access keys and roles. The test running once withdeployer_as_super_admin()is sufficient, as it covers all relevant relayer key scenarios. The parameterization concern reflects a misunderstanding of whatno_registrationcontrols within this codebase.tests/src/tests/defuse/intents/simulate.rs (1)
45-45: The review comment raises a valid concern about test coverage, but the claimed code simplification cannot be verified in the current codebase.The
disable_registrationmechanism does affect token behavior (when enabled, it restrictsstorage_deposit()calls to owner/contract only), so hypothetically, testing with different registration modes would provide different coverage. However:
No evidence of the claimed simplification: All current simulate tests use
Env::builder().build().awaitwith default registration settings (allowing self-registration). No tests in the codebase currently use.no_registration(true).No git history of recent changes: The simulate.rs file shows no recent modifications removing explicit registration configuration.
The mechanism is real: Registration mode DOES affect
storage_deposit()behavior in poa-token contracts (lines 191-197 of poa-token/src/contract/mod.rs), and asimulate_storage_deposit_intent()test exists.Missing test variant: If coverage is a concern, there's no evidence of a parallel test using
Env::builder().no_registration(true).build().awaitto verify simulation behavior with restricted registration.To properly assess coverage impact, verify whether an earlier version of these tests explicitly used
.no_registration(true)and what specific test assertions differed.poa-factory/src/lib.rs (2)
11-19: LGTM! Well-documented trait for per-token access key management.The
TokenFullAccessKeystrait provides a clean interface for managing full access keys on individual token contracts. The security notes about requiring 1 yⓃ attachment are appropriate for preventing unauthorized key manipulation.
25-30: Review comment is accurate and verified.All claims in the original review are substantiated:
- ✅ The trait signature uses
Option<bool>forno_registration(poa-factory/src/lib.rs:29)- ✅ The implementation handles
Nonewith an appropriate default:.unwrap_or(DEFAULT_NO_REGISTRATION)(poa-token/src/contract/mod.rs:69)- ✅ The default value is sensible:
DEFAULT_NO_REGISTRATION = true(poa-token/src/contract/state.rs:14)- ✅ Backward compatibility is maintained: callers can omit the parameter and it applies the default
Makefile.toml (2)
19-26: Verify intentional difference between default and test builds.The default
buildtask usesbuild-poa-factory-global(withglobal_contractsfeature), whiletest-buildusesbuild-poa-factory(without the feature). Ensure this is intentional and tests don't require the global contracts functionality.
162-166: Good addition of dedicated test-build task.Separating test build dependencies from test execution improves build clarity and allows for more granular control over what gets built for tests.
tests/src/tests/defuse/intents/token_diff.rs (2)
264-265:invariant_violatedtest now uses default environment.The test no longer parameterizes
no_registration, simplifying the test setup while retaining the core invariant violation logic.
20-23: The review comment's premise is incorrect—these tests never usedno_registrationparameterization.Verification shows that
swap_p2p,swap_many,invariant_violated, andsolver_user_closureintoken_diff.rshave no history of using ano_registrationparameter. Git history and current code inspection confirm these tests only vary by thefeeparameter via#[values(...)]. Theno_registrationoption remains available inEnvBuilderfor other tests (e.g., inpoa/factory.rsandft_withdraw.rs), so this is not a removal of a contract feature.Likely an incorrect or invalid review comment.
tests/src/tests/defuse/tokens/nep245/mod.rs (2)
31-36: Test simplified to use default environment configuration.The
multitoken_enumerationtest now uses a fixed environment setup withcreate_unique_users(), removing theno_registrationparameter variation.
319-322: Consistent simplification pattern applied.The
multitoken_enumeration_with_rangestest follows the same simplification pattern, maintaining consistency across the test suite.poa-token/src/contract/state.rs (3)
14-14: Default value forno_registrationis documented clearly.The constant
DEFAULT_NO_REGISTRATION = trueprovides a clear default for legacy state migration and new deployments.
16-46: Well-structured versioned state migration pattern.The
VersionedStateenum withCowfor zero-copy serialization and the conversion implementations provide a clean upgrade path from legacy (StateV0) to the current state format. The comment about Borsh always deserializing intoCow::Ownedis helpful.
71-109: Robust versioned deserialization with magic prefix.The magic prefix approach (
u32::MAX) for distinguishing versioned vs legacy state is sound. The comment explaining why legacy contracts won't start with this prefix (due toLookupMapprefix bytes) provides good justification for the safety assumption.tests/src/tests/poa/factory.rs (5)
30-43: LGTM!The addition of the
no_registrationparameter to both trait methods is clean and consistent. The explicit boolean parameter forces callers to make a conscious decision about registration requirements.
102-132: LGTM!The implementation correctly propagates the
no_registrationparameter through the JSON RPC call and forwards it appropriately in the convenience wrapper method.
195-216: LGTM!The Contract implementation correctly delegates to the Account implementation while properly forwarding the
no_registrationparameter.
287-300: LGTM!The existing test correctly maintains its original behavior by passing
falseforno_registration, ensuring backward compatibility and consistent test coverage for the standard registration flow.
328-397: Perfect! I have now completed a thorough verification of the storage_deposit access control test. Let me provide the final analysis:Verification Summary:
After tracing through the code:
Token Owner Assignment: When the factory deploys a token with
no_registration=true, the factory calls the token'snew()method WITHOUT passing anowner_idparameter. The token'snew()method defaults the owner toPREDECESSOR_ACCOUNT_IDat that moment, which is the factory contract itself (factory.id()), NOTroot.Storage Deposit Access Control: The token's
storage_depositmethod (lines 191-197 in poa-token/src/contract/mod.rs) requires:
- When
no_registration=true, only the owner (factory) or the token contract itself can call itTest Execution: In the test,
root.ft_storage_deposit(&ft1, None)makes a call where:
PREDECESSOR_ACCOUNT_ID=root.id()- Token owner =
factory.id()- Token contract =
ft1root.id()is neither the owner nor the contract itself- Therefore, the call correctly fails with "Method is private"
The review comment's analysis is incorrect. The test expectation at lines 376-383 is valid and correct.
Verify storage_deposit access control test expectation.
The test expectation at lines 376-383 is correct. When the factory deploys a token with
no_registration=true, the token's owner is set to the factory contract (notroot). Therefore,root.ft_storage_deposit(&ft1, None)correctly fails with "Method is private" becauserootis neither the owner nor the contract itself.poa-token/src/contract/mod.rs (6)
1-24: LGTM!The new imports are properly organized and support the state versioning and serialization refactoring introduced in this file.
34-43: LGTM!The refactoring to use a versioned
Statewrapper with custom serialization and auto-deref is a solid pattern for enabling contract state migrations while maintaining backward compatibility.
47-86: LGTM!The constructor correctly initializes the new
Statestructure with theno_registrationparameter, providing a sensible default viaDEFAULT_NO_REGISTRATIONwhen the parameter is not supplied.
88-101: LGTM!The migration function correctly implements the pattern for upgrading from
StateV0toState, with appropriate security controls (privateandassert_one_yocto). Theinit(ignore_state)attribute is essential for the migration to work properly.
186-199: LGTM!The access control for
storage_depositcorrectly restricts the method to the owner or the contract itself whenno_registrationis enabled, which aligns with the design intent of preventing external registration in no-registration mode.
259-271: LGTM!The
NoRegistrationtrait implementation provides appropriate getter/setter methods with proper access control. Theset_no_registrationmethod correctly restricts changes to the owner and requires one yocto for security.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
poa-factory/src/contract.rs (1)
104-149: PoA tokennewsignature mismatch:owner_idparameter missing from factory's JSON payload.The factory's
deploy_tokenfunction (lines 105-149) sends a JSON payload with onlymetadataandno_registration, but the PoA token'snewinitializer atpoa-token/src/contract/mod.rs:49-53expectsowner_idas the first parameter:// poa-token/src/contract/mod.rs:49-53 pub fn new( owner_id: Option<AccountId>, metadata: Option<FungibleTokenMetadata>, no_registration: Option<bool>, ) -> SelfThe factory sends:
{ "metadata": metadata, "no_registration": no_registration }The factory needs to include
owner_idin the JSON payload passed to thenewcall (likelyself.predecessor_account_id()or similar). Without it, deserialization will fail and initialization will panic.
🧹 Nitpick comments (2)
poa-factory/src/contract.rs (1)
4-4: Consider requiring known tokens before managing per‑token full‑access keys.The new
TokenFullAccessKeysimpl and use ofext_full_access_keysare straightforward and mirror the global key APIs, but unlikeset_metadata/ft_deposityou don’t currently assert that thetokenexists inself.tokensbefore calling out. If you want these methods to operate only on factory‑managed tokens and keep semantics consistent, you could add arequire!(self.tokens.contains(&token), "token does not exist");check to both methods before constructing the promise.fn add_token_full_access_key(&mut self, token: String, public_key: PublicKey) -> Promise { assert_one_yocto(); + require!(self.tokens.contains(&token), "token does not exist"); ext_full_access_keys::ext(Self::token_id(token)) .with_attached_deposit(NearToken::from_yoctonear(1)) .add_full_access_key(public_key) } fn delete_token_full_access_key(&mut self, token: String, public_key: PublicKey) -> Promise { assert_one_yocto(); + require!(self.tokens.contains(&token), "token does not exist"); ext_full_access_keys::ext(Self::token_id(token)) .with_attached_deposit(NearToken::from_yoctonear(1)) .delete_key(public_key) }Also applies to: 22-22, 246-265
poa-token/src/lib.rs (1)
15-22: Consider clarifying the doc comment.The doc comment "whether the token allows accounts initialize storage deposits by itself" is grammatically awkward. Consider rephrasing to clarify the meaning—e.g., "Returns whether the token requires users to register storage (false) or handles it internally (true)."
pub trait NoRegistration { - /// Returns whether the token allows accounts initialize storage deposits by itself. + /// Returns whether user-initiated storage registration is disabled. + /// When `true`, only the owner or the contract itself can call `storage_deposit`. fn no_registration(&self) -> bool;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
poa-factory/src/contract.rs(5 hunks)poa-token/src/contract/mod.rs(8 hunks)poa-token/src/contract/state.rs(1 hunks)poa-token/src/lib.rs(2 hunks)tests/src/tests/defuse/intents/ft_withdraw.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/tests/defuse/intents/ft_withdraw.rs
🧰 Additional context used
🧬 Code graph analysis (3)
poa-token/src/lib.rs (1)
poa-token/src/contract/mod.rs (2)
no_registration(259-261)set_no_registration(265-268)
poa-factory/src/contract.rs (3)
poa-factory/src/lib.rs (1)
deploy_token(25-30)poa-token/src/contract/mod.rs (1)
no_registration(259-261)poa-token/src/lib.rs (1)
no_registration(17-17)
poa-token/src/contract/state.rs (1)
poa-token/src/contract/mod.rs (2)
ft_balance_of(162-164)new(49-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Security Audit - report
🔇 Additional comments (9)
poa-factory/src/contract.rs (1)
24-30: ConfirmPOA_TOKEN_INIT_BALANCEvalues and comment underglobal_contracts.The cfg split between
from_millinear(5)forglobal_contractsandfrom_near(3)otherwise looks reasonable, and gatingPOA_TOKEN_WASMbehindnot(global_contracts)is clean. It might be worth double‑checking that the 5 millinear initial balance still comfortably covers per‑token state for the global-contracts path, and updating the “size ~ 500B” comment if that assumption has changed.Also applies to: 36-38
poa-token/src/contract/state.rs (3)
71-84: Well-designed magic prefix for version detection.The use of
u32::MAXas a magic prefix is a clever approach to distinguish between legacy and versioned contract state during deserialization. The comment correctly explains why this is safe—legacy contracts would have a different byte pattern in the first 4 bytes due to theLookupMapprefix.
86-110: Deserialization logic is sound.The fallback path correctly prepends the already-consumed 4 bytes using
buf.chain(reader)before deserializing as legacyStateV0. The chained.map(Into::into)calls properly convert throughVersionedStatetoState.
231-241: Test correctly exercises the legacy upgrade path.The test properly validates the upgrade flow: serialize
StateV0directly (simulating legacy format), then deserialize throughContract(which usesMaybeVersionedContractState), and verify data integrity. Thedrop(legacy_acc)is correctly placed to ensure near-sdk collections flush to storage before deserialization.poa-token/src/lib.rs (1)
28-37: Adding supertrait is appropriate for the ext_contract pattern.The addition of
NoRegistrationtoPoaFungibleTokensupertrait bounds is consistent with the contract's extended capabilities. Since this is anext_contract, it primarily defines the cross-contract call interface rather than expecting external implementations.poa-token/src/contract/mod.rs (4)
35-43: Clean pattern using autoimpl for state access.The use of
autoimpl(Deref using self.state)andautoimpl(DerefMut using self.state)combined with custom borsh serialization viaMaybeVersionedContractStateis a clean approach. This allows transparent access toStatefields while maintaining versioned (de)serialization.
257-269: NoRegistration implementation looks correct.The getter properly leverages the
Derefautoimpl to accessState.no_registration. The setter is appropriately protected with#[only(self, owner)],#[payable], andassert_one_yocto()to prevent accidental or unauthorized changes.
49-71: Constructor correctly handles the new no_registration parameter.The constructor properly accepts an optional
no_registrationflag with a sensible default (DEFAULT_NO_REGISTRATION). TheStateinitialization is clean and consistent.
88-101: I'll verify the concerns raised in this review comment about themigratefunction. Let me start by exploring the codebase structure and examining the state management implementation.
<function_calls>
find . -name "mod.rs" -type f | head -10
</function_calls>
<function_calls>
find . -name "state.rs" -type f
</function_calls>
<function_calls>
find . -type f -name "*.rs" | grep -E "(contract|state)" | head -20
</function_calls>Let me search more broadly for the contract file and state definitions:
<function_calls>
rg -l "migrate" --type rust | head -10
</function_calls>Now let me examine the actual migrate function and related state structures:
<function_calls>
rg -n "fn migrate" -A 15 --type rust
</function_calls>Let me also search for MaybeVersionedContractState and state versioning:
<function_calls>
rg -n "MaybeVersionedContractState|VERSIONED_MAGIC_PREFIX|StateV0|STATE" --type rust -C 2 | head -80
</function_calls>Let me check the repository structure first:
<function_calls>
ls -la poa-token/src/contract/ 2>/dev/null || ls -la poa-token/ 2>/dev/null || find . -type d -name "poa-token" | head -5
</function_calls><function_calls>
git ls-files | grep -E "poa-token.*mod.rs|poa-token.*state.rs" | head -10
</function_calls><function_calls>
git ls-files | grep ".rs$" | head -30
</function_calls>Let me check the PR context to find the actual file paths:
<function_calls>
rg -l "pub fn migrate" --type rust
</function_calls><function_calls>
rg -n "StateV0" --type rust -B 3 -A 3
</function_calls>Let me try a broader search:
<function_calls>
find . -name "*.rs" -type f | head -40
</function_calls><function_calls>
cat -n poa-token/src/contract/mod.rs 2>/dev/null | sed -n '80,110p'
</function_calls>Now let me examine the state structure:
<function_calls>
find . -path "poa-token" -name "*.rs" -type f
</function_calls><function_calls>
rg -l "struct State|pub enum MaybeVersioned" --type rust
</function_calls>Let me search for the state definitions directly:
<function_calls>
rg -n "struct StateV0|struct State" --type rust -A 5
</function_calls>Let me try to understand the file structure:
<function_calls>
ls -la poa-token/src/
</function_calls><function_calls>
cat poa-token/src/contract/mod.rs | head -120 | tail -40
</function_calls>Now let me check if there's any state versioning mechanism:
<function_calls>
cat poa-token/src/state.rs 2>/dev/null | head -100
</function_calls><function_calls>
rg -n "env::state_read|env::storage_read" --type rust -B 2 -A 2
</function_calls>Let me examine the full migrate function and surrounding context:
<function_calls>
cat poa-token/src/contract/mod.rs | head -150 | tail -70
</function_calls>Now let me check the imports and the state definition:
<function_calls>
head -50 poa-token/src/contract/mod.rs
</function_calls><function_calls>
cat poa-token/src/state.rs
</function_calls>Perfect! Now I have the full context. Let me search for any version checking or magic prefix patterns:
<function_calls>
rg -n "0xFFFFFFFF|MAGIC|VERSIONED" --type rust
</function_calls>Let me check how near-sdk handles state versioning:
<function_calls>
cat poa-token/Cargo.toml | grep -A 5 near-sdk
</function_calls>Now let me search for any initialization or migration patterns in the contract:
<function_calls>
rg -n "init|new|contract(" --type rust poa-token/src/contract/mod.rs | head -20
</function_calls>Let me check if the code has any actual tests or usage patterns for migrate:
<function_calls>
find . -path "poa-token" -name "*.rs" -type f | xargs rg -l "migrate"
</function_calls><function_calls>
cat poa-token/src/contract/mod.rs | sed -n '1,200p' | tail -100
</function_calls>Let me check what the #[init] decorator does:
<function_calls>
near-sdk #[init(ignore_state)] decorator rust contract migration
</function_calls>Let me also check if there's existing state versioning in the codebase:
<function_calls>
rg -n "impl.*From.*StateV0|into|from_slice" --type rust poa-token/ -C 2
</function_calls>Now let me understand the actual issue by looking at what happens when env::state_read is called:
<function_calls>
cat poa-token/src/contract/mod.rs | sed -n '85,105p'
</function_calls>Perfect! Now let me verify the critical claim about the review comment - whether there actually is a MaybeVersionedContractState or any versioning mechanism:
<function_calls>
rg -n "MaybeVersioned|Versioned" --type rust poa-token/
</function_calls>Let me check for any version handling in the From implementation:
<function_calls>
cat poa-token/src/state.rs | grep -n "From|impl" -A 10 | head -40
</function_calls>Now let me understand the actual structure of the contract initialization:
<function_calls>
rg -n "#[contract]|pub struct|#[init]" --type rust poa-token/src/contract/mod.rs -B 1 -A 3
</function_calls>Let me check if there are other places where state is read:
<function_calls>
rg -n "state_read|storage_read|"STATE"" --type rust poa-token/
</function_calls>Let me verify the actual near-sdk version and understand how state management works:
<function_calls>
cat poa-token/Cargo.toml
</function_calls>Now let me check how #[init(ignore_state)] actually works - does it prevent re-initialization:
<function_calls>
"ignore_state" near-sdk rust contract initialization
</function_calls>Let me look for any test files or documentation:
<function_calls>
find . -path "poa-token" -type f ( -name ".md" -o -name ".txt" ) | head -10
</function_calls>Let me check if there's any runtime state serialization/deserialization that would use a magic prefix:
<function_calls>
rg -n "borsh|serialize|deserialize" --type rust poa-token/src/ | head -20
</function_calls>Now let me verify if the migrate function could actually be called twice:
<function_calls>
cat poa-token/src/contract/mod.rs | grep -n "migrate|#[payable]|#[private]|#[init" -B 2 -A 1
</function_calls>Let me check if #[init] with ignore_state prevents double initialization:
<function_calls>
rg -n "impl.*Contract|pub fn.*new|pub fn.*init" --type rust poa-token/src/contract/mod.rs -B 2 -A 8 | head -50
</function_calls>Let me examine the entire contract definition:
<function_calls>
wc -l poa-token/src/contract/mod.rs
</function_calls><function_calls>
cat poa-token/src/contract/mod.rs
</function_calls>Perfect! Now I have the full picture. Let me verify key claims about the review comment:
Now let me verify one more time that MaybeVersionedContractState truly doesn't exist:
<function_calls>
rg -r "MaybeVersioned|0xFFFFFFFF" --type rust .
</function_calls>Let me also check the near-sdk documentation to understand env::state_read behavior:
<function_calls>
near-sdk env::state_read borsh deserialization how it works
</function_calls>Now let me verify if there's any actual risk with calling migrate twice:
<function_calls>
rg -n "assert|require|panic" --type rust poa-token/src/contract/mod.rs | head -20
</function_calls>Let me check what happens when #[init(ignore_state)] is used:
<function_calls>
near-sdk rust "ignore_state" init decorator prevents double initialization
</function_calls>Based on my thorough investigation of the codebase and verification of the claims made in the review comment, I can now provide my final assessment:
The review comment contains fundamental inaccuracies and references non-existent code constructs.
The core issue: The review comment proposes a fix that references
MaybeVersionedContractState::VERSIONED_MAGIC_PREFIXand suggests usingenv::storage_read()with specific magic byte checking, but neitherMaybeVersionedContractStatenor any versioning mechanism with magic prefixes (0xFFFFFFFF) exists in this codebase. These are fabrications not present in the actual contract implementation.Key findings from verification:
- Non-existent code references:
MaybeVersionedContractState,VERSIONED_MAGIC_PREFIX, and the magic prefix pattern0xFFFFFFFFdo not exist anywhere in the codebase- Incorrect understanding of near-sdk: The codebase uses standard near-sdk Borsh serialization, not a custom versioning adapter
- Misdiagnosed risk: The
#[init(ignore_state)]decorator specifically allows custom state loading logic, and combined with#[private], provides adequate protection against misuse- Non-compiling fix: The proposed code would not compile as it references undefined types and constants
Likely an incorrect or invalid review comment.
pityjllk
left a comment
pityjllk
left a comment
There was a problem hiding this comment.
lgtm, few minor comments
| /// Copied from `near_contract_standards::fungible_token::core_impl::GAS_FOR_FT_TRANSFER_CALL` | ||
| const POA_TOKEN_FT_TRANSFER_CALL_MIN_GAS: Gas = Gas::from_tgas(30); | ||
|
|
||
| #[cfg(not(feature = "global_contracts"))] |
There was a problem hiding this comment.
do we even need to support non global contracts? maybe we should simply get rid of it?
There was a problem hiding this comment.
Near workspace does not support global contracts, so I have to add it as a feature for now - it is not used in tests to be compatible with near workspace (ci builds it also without this feature)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
poa-token/src/contract/mod.rs (1)
188-195: Avoid panic when ownership is renounced whileno_registrationis trueIf ownership is renounced (
owner_get()returnsNone) whileno_registrationistrue, the currentunwrap_or_else(|| unreachable!())will panic instead of cleanly enforcing access control. TreatingNoneas “only the contract itself may call” is safer and matches the intent.Consider:
- if self.no_registration { - require!( - *PREDECESSOR_ACCOUNT_ID == self.owner_get().unwrap_or_else(|| unreachable!()) - || *PREDECESSOR_ACCOUNT_ID == *CURRENT_ACCOUNT_ID, - "Method is private" - ); - } + if self.no_registration { + require!( + self + .owner_get() + .map_or(false, |owner| *PREDECESSOR_ACCOUNT_ID == owner) + || *PREDECESSOR_ACCOUNT_ID == *CURRENT_ACCOUNT_ID, + "Method is private" + ); + }This way, after ownership is renounced, only the contract itself can call
storage_depositwhenno_registrationis enabled.
🧹 Nitpick comments (2)
poa-token/src/contract/mod.rs (2)
88-100: Migration flow fromStateV0toStatelooks standard; verify layout before running in prodReading the old state with
env::state_read::<StateV0>(), converting intoState, and explicitly settingno_registrationis the usual NEAR upgrade pattern; just make sureStateV0’s Borsh layout matches the previous contract state and thatmigrateis only invoked once right after upgrading the code.
256-268:NoRegistrationimplementation is consistent; consider emitting an event on changeThe getter correctly exposes the flag and the owner-gated, 1-yocto
set_no_registrationmutator updates the same field used instorage_deposit, so semantics are coherent; as an optional enhancement, you might emit a small config-change event fromset_no_registrationso indexers can track when registration is enabled/disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
poa-token/src/contract/mod.rs(8 hunks)poa-token/src/contract/state.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- poa-token/src/contract/state.rs
🧰 Additional context used
🧬 Code graph analysis (1)
poa-token/src/contract/mod.rs (1)
poa-token/src/lib.rs (2)
no_registration(17-17)set_no_registration(21-21)
🔇 Additional comments (4)
poa-token/src/contract/mod.rs (4)
2-4: Versioned state imports and module exposure look consistentBringing in the adapter/types and exposing the
statemodule (NoRegistration,PoaFungibleToken,DEFAULT_NO_REGISTRATION,MaybeVersionedContractState,State,StateV0) matches the new versioned-state + no-registration design; nothing concerning here.Also applies to: 19-24
35-42: Deref delegation to innerStateplus custom serde is coherentForwarding
Deref/DerefMuttoself.stateand annotatingstate: Statewith a custom (de)serializer cleanly centralizes persisted state while keeping the rest of the impls largely unchanged; this is a solid layout for future migrations.
49-53: Initializer now wiresno_registrationinto persisted state cleanlyExtending
newwithno_registration: Option<bool>and defaulting viaDEFAULT_NO_REGISTRATIONwhenNonemakes the flag explicit in state without breaking callers; theStateconstruction (token, metadata, no_registration) looks correct.Also applies to: 66-70
118-125: No functional change in this hunkThis appears to be a whitespace-only tweak around the existing
FtMintemission inft_deposit; no behavioral differences to act on here.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile.toml(6 hunks)
🔇 Additional comments (1)
Makefile.toml (1)
78-93: Verify the manifest path forbuild-poa-token.The AI summary states: "build-poa-token with manifest path ./poa-factory/Cargo.toml", but line 87 specifies
./poa-token/Cargo.toml. This discrepancy needs clarification—confirm whether the token should be built from the token crate or the factory crate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/src/tests/defuse/intents/token_diff.rs (1)
262-266: Fix attribute order for consistency with other tests.The attribute order here is
#[tokio::test]followed by#[rstest], which is reversed compared to the other test functions in this file (lines 17-18, 85-86, 360-361). For consistency and to follow rstest best practices,#[rstest]should be the outermost attribute (first).Additionally, since this test function has no parameters, the
#[rstest]attribute is unnecessary and can be removed entirely.Apply this diff to fix the attribute order:
-#[tokio::test] -#[rstest] +#[rstest] +#[tokio::test] #[trace] async fn invariant_violated() {Or alternatively, remove the unnecessary
#[rstest]attribute:#[tokio::test] -#[rstest] #[trace] async fn invariant_violated() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(1 hunks)tests/src/tests/defuse/env/mod.rs(2 hunks)tests/src/tests/defuse/env/storage.rs(1 hunks)tests/src/tests/defuse/intents/ft_withdraw.rs(7 hunks)tests/src/tests/defuse/intents/legacy_nonce.rs(1 hunks)tests/src/tests/defuse/intents/mod.rs(3 hunks)tests/src/tests/defuse/intents/public_key.rs(2 hunks)tests/src/tests/defuse/intents/simulate.rs(6 hunks)tests/src/tests/defuse/intents/token_diff.rs(4 hunks)tests/src/tests/defuse/storage/mod.rs(2 hunks)tests/src/tests/defuse/tokens/nep141/mod.rs(6 hunks)tests/src/tests/defuse/tokens/nep245/mod.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- Cargo.toml
- tests/src/tests/defuse/env/storage.rs
- tests/src/tests/defuse/intents/mod.rs
- tests/src/tests/defuse/intents/public_key.rs
- tests/src/tests/defuse/intents/ft_withdraw.rs
- tests/src/tests/defuse/tokens/nep245/mod.rs
- tests/src/tests/defuse/storage/mod.rs
- tests/src/tests/defuse/tokens/nep141/mod.rs
🧰 Additional context used
🧬 Code graph analysis (3)
tests/src/tests/defuse/intents/simulate.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/intents/legacy_nonce.rs (1)
tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
tests/src/tests/defuse/intents/token_diff.rs (2)
tests/src/tests/defuse/env/builder.rs (1)
fee(40-43)tests/src/tests/defuse/env/mod.rs (1)
builder(58-60)
⏰ 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 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: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (6)
tests/src/tests/defuse/intents/token_diff.rs (1)
20-23: LGTM! Test parameterization and setup simplified correctly.The function signature now uses rstest's
#[values(...)]attribute to parameterize the fee, and the environment setup has been simplified by removing theno_registrationbranch. This aligns with the PR objectives to simplify test scaffolding.tests/src/tests/defuse/intents/simulate.rs (2)
44-44: Unified Env initialization across simulate_ tests looks good*Using
Env::builder().build().awaitin these tests standardizes setup and removes the special-case no-registration plumbing without changing the test intent. No issues from this change.Also applies to: 108-108, 176-176, 258-258, 489-489
568-568: Zero‑fee Env configuration for token‑diff simulation is preserved
Env::builder().fee(Pips::ZERO).build().awaitkeeps the explicit zero‑fee configuration forsimulate_token_diff_intentwhile aligning with the new Env builder API. The expectedfees_collected: Amounts::default()assertions should still hold.tests/src/tests/defuse/intents/legacy_nonce.rs (1)
18-18: Legacy nonce test now uses the standard Env builder pathSwitching to
Env::builder().build().awaitaligns this test with the rest of the suite and removes bespoke no‑registration wiring. GivenEnv::builder()is justEnvBuilder::default(), behavior now hinges on those defaults; as long as they match the previous expectations for legacy nonce handling, this looks correct.tests/src/tests/defuse/env/mod.rs (2)
27-27: near_workspaces import narrowing is appropriateImporting only
AccountandContractmatches this module’s usage and removes unused symbols; no functional change here.
107-110: create_named_token correctly forwards disable_registration into PoA factoryRouting
self.disable_registrationintopoa_factory_deploy_tokenconsolidates registration behavior at the factory level and replaces the old no‑registration deployment branch, in line with the PR’s goals. The.await.unwrap()pattern is consistent with the rest of the test harness. Please just confirm thatEnvBuilderinitializesdisable_registrationin a way that preserves prior defaults for tests that did or did not opt into no‑registration explicitly.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.