-
Notifications
You must be signed in to change notification settings - Fork 9
feat: track locked nep141 tokens #481
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
kiseln
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about transfers to other chain? We need to subtract tokens for source and add for destination
It's safer to decrease locked tokens in first callback and increase back if refund is needed
Added in 057401e |
| utxo_chain_connectors: old_state.utxo_chain_connectors, | ||
| migrated_tokens: LookupMap::new(StorageKey::MigratedTokens), | ||
| migrated_tokens: old_state.migrated_tokens, | ||
| locked_tokens: LookupMap::new(StorageKey::LockedTokens), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to pause a contract, apply migration first and unpause it later. It might be better to allow providing a vec of locked tokens for migrate methods. Which option is better?
P.S: technically if locked_tokens field is empty, then it can be considered as a paused contract already, since init_transfer, fin_transfer methods won't work until locked_tokens are populated properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements per-chain locked-token accounting on the NEAR bridge to track NEP-141 tokens locked for each destination chain. The bridge now maintains a locked_tokens map indexed by (chain, token), incrementing balances when transferring from NEAR to other chains and enforcing availability before releasing tokens back to NEAR recipients.
Changes:
- Added
locked_tokensfield to track token balances per chain - Implemented increase/decrease operations with overflow and insufficient balance checks
- Updated transfer flows to maintain locked token accounting
- Added administrative
set_locked_tokensfunction for manual backfilling
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| near/omni-bridge/src/lib.rs | Core implementation of locked tokens tracking with get/set/increase/decrease operations integrated into transfer flows |
| near/omni-bridge/src/migrate.rs | Migration logic that initializes locked_tokens as empty map requiring manual backfilling |
| near/omni-bridge/src/tests/lib_test.rs | Unit tests for locked token tracking including insufficient balance and refund scenarios |
| near/omni-tests/src/init_transfer.rs | Updated migration test to verify locked_tokens functionality |
| near/omni-tests/src/environment.rs | Test environment setup with locked token seeding for new deployments |
| near/Cargo.toml | Version bump to 0.4.4 |
| near/Cargo.lock | Dependency updates (various minor version bumps) |
Comments suppressed due to low confidence (1)
near/omni-tests/src/init_transfer.rs:716
- The migration test now only verifies that locked_tokens can be queried after migration, but it no longer verifies that existing pending_transfers are preserved during the migration. The previous test performed an init_transfer before migration and then verified the transfer message was still accessible afterwards. Consider adding a verification that existing state (pending_transfers, finalized_transfers, etc.) is properly preserved through the migration to ensure no data loss occurs.
#[rstest]
#[tokio::test]
async fn test_migrate(build_artifacts: &BuildArtifacts) -> anyhow::Result<()> {
let sender_balance_token = 1_000_000;
let env = TestEnv::new(sender_balance_token, true, build_artifacts).await?;
let res = env
.locker_contract
.as_account()
.deploy(&build_artifacts.locker)
.await
.unwrap();
assert!(res.is_success(), "Failed to upgrade locker");
let res = env
.locker_contract
.call("migrate")
.max_gas()
.transact()
.await?;
assert!(res.is_success(), "Migration didn't succeed");
let transfer = env
.locker_contract
.call("get_locked_tokens")
.args_json(json!({
"chain_kind": ChainKind::Near,
"token_id": env.token_contract.id(),
}))
.max_gas()
.transact()
.await?;
assert_eq!(
transfer.into_result()?.json::<U128>()?,
U128(0),
"Locked tokens not migrated correctly"
);
Ok(())
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There's a case where user can call update_transfer_fee, but use previous sign, therefore previous logic of locking always net amount without fee is invalid. It's safer to unlock in claim_fee
62b8ba2 to
b511f77
Compare
e845851 to
746df46
Compare
11002c0 to
0c15ae4
Compare
0c15ae4 to
a73a8ee
Compare
olga24912
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t see lock_token in the function utxo_fin_transfer_to_other_chain
| } | ||
|
|
||
| #[access_control_any(roles(Role::DAO))] | ||
| pub fn set_locked_tokens(&mut self, chain_kind: ChainKind, token_id: AccountId, amount: U128) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is needed for a migration, do we want to accept an array with all the data right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edcc502 to
e836153
Compare
Fixed in e836153 |
d77f951 to
05743c0
Compare
Implemented per-chain locked-token accounting on the NEAR bridge side and enforced it when releasing tokens. The contract now tracks locked balances by
(chain, token), increments them on NEAR-origin transfers, and requires/consumes them before unlocking to NEAR recipientsNote
Deploying this requires backfilling initial locked token amounts—reindex prior bridge events to populate locked_tokens before allowing withdrawals.
This PR introduces a new mechanism for tracking and managing locked tokens in the
omni-bridgecontract, adds administrative methods for managing locked tokens, and refactors several parts of the transfer logic to ensure proper token locking and unlocking during cross-chain operations. It also updates the state migration logic and makes several improvements to fee handling and transfer normalization.Token Locking Mechanism:
locked_tokensfield to theContractstruct, backed by aLookupMap, and the associatedStorageKey::LockedTokensvariant for storage management. This allows the contract to track the amount of tokens locked per(ChainKind, AccountId)pair. [1] [2] [3]lock_tokens_if_neededandunlock_tokens_if_neededto manage token locking, ensuring tokens are locked when bridging to non-NEAR, non-UTXO chains and unlocked when appropriate. These are used throughout the transfer and finalization logic to prevent double-minting or unauthorized release of tokens. [1] [2] [3] [4]Administrative and Query Methods:
get_locked_tokens(read-only) andset_locked_tokens(restricted to DAO role) for querying and administratively setting locked token amounts for a given chain and token.Transfer and Fee Handling Refactor:
amount_without_feemethod for calculating transfer amounts, improving clarity and reducing potential for errors. [1] [2] [3]State Migration Improvements:
locked_tokensandmigrated_tokensfields, as well asfinalised_utxo_transfers, ensuring seamless upgrades from previous contract versions. [1] [2] [3]Other Improvements:
0.4.3to0.4.4inCargo.toml.These changes collectively improve the security and correctness of token bridging, especially in scenarios involving fast transfers and cross-chain operations.