feat: require deposits for a few migration functions by gilcu3 · Pull Request #3874 · near/mpc

gilcu3 · 2026-07-21T07:01:38Z

I added deposits for the functions that are started by operators independently to make sure function-call keys cannot call such functions. For conclude_node_migration it was not added as this is currently called directly by the node, and should be running inside a TEE anyway.

Some of these deposits might need to be removed after the full backup service is implemented, but for now it is good to have them as defense in depth.

claude · 2026-07-21T07:21:44Z

Since this PR changes source code behavior by adding deposit requirements to contract functions, the type should probably be feat: instead of chore:.

Suggested title: feat: require deposits for a few migration functions

claude · 2026-07-21T07:24:25Z

Pull request overview

Adds a 1 yoctoNEAR deposit requirement (via #[payable] + require_deposit) to three operator-initiated contract methods — register_backup_service, start_node_migration, update_participant_url — so a leaked node function-call access key cannot invoke them (function-call keys cannot attach a deposit). Updates CLI helpers, e2e tests, unit/integration tests, and the operator docs to match; the ABI snapshot is refreshed.

Changes:

New MINIMUM_NODE_MANAGEMENT_DEPOSIT constant and #[payable] + require_deposit(...) on the three methods in crates/contract/src/lib.rs
Environment test helper gains an attached_deposit field with set_deposit(); new unit tests for the no-deposit rejection path
New participant_context_with_deposit helper and a rejection integration test for update_participant_url
E2E cluster.rs switches the three helpers to call_from_deposit(...) with a 1 yoctoNEAR attachment
Docs and backup-cli command hints updated to attach 1 yoctoNEAR instead of 0 NEAR
ABI snapshot refreshed (payable modifier + updated doc comments)

Reviewed changes

Per-file summary

File	Description
crates/contract/src/lib.rs	Adds `MINIMUM_NODE_MANAGEMENT_DEPOSIT`, `#[payable]` and `require_deposit(...)` calls in the three methods; updates doc comments; adds two `should_panic` unit tests; adjusts existing success tests to attach a deposit.
crates/contract/src/tee/test_utils.rs	Adds a `deposit` field to `Environment` with `set_deposit()`; refactors `new()` to reuse `set()`.
crates/contract/tests/inprocess/common.rs	Adds `participant_context_with_deposit` helper for payable-method callers.
crates/contract/tests/inprocess/update_participant_url.rs	Uses the new helper for the success case; adds a rejection test for zero deposit.
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap	Reflects payable modifiers and updated docstrings for the three methods.
crates/e2e-tests/src/blockchain.rs	Adds `call_from_deposit(...)` convenience wrapper.
crates/e2e-tests/src/cluster.rs	Switches the three cluster helpers to attach `NODE_MANAGEMENT_DEPOSIT`.
crates/backup-cli/src/backup.rs	Updates printed `register_backup_service` command to attach 1 yoctoNEAR.
docs/node-migration-guide.md	Updates `attached-deposit` in the operator-facing examples.

Findings

Blocking (must fix before merge):

None.

Non-blocking (nits, follow-ups, suggestions):

crates/contract/src/lib.rs:2494, :2536, :2568 — require_deposit is placed after the participant/state validation in all three methods, while the pre-existing pattern in assert_valid_signature_request_and_charge (see crates/contract/src/lib.rs:305) checks the deposit early. Current ordering is what makes tests like test_start_node_migration_failure_not_participant (:4641) and test_register_backup_service_fail_non_participant (:4729) return NotParticipant instead of panicking on deposit. Functionally fine, but worth a brief comment explaining the intentional ordering — otherwise the next reader may "cleanup" by hoisting require_deposit and silently break those tests.
crates/contract/src/lib.rs:2470-2472 — the note about the future hard-launch flow revisiting this requirement is real context, but per [engineering-standards.md §Write helpful code comments (5)] this kind of forward-looking rationale is a better fit for a TODO(#issue): ... linked to the tracking issue than a permanent doc-comment paragraph on the method (which will end up in ABI and docs).
crates/contract/src/lib.rs:4851, :4874, and crates/contract/tests/inprocess/update_participant_url.rs:80 — the // panics via require_deposit before the ... is stored trailing comments paraphrase what the #[should_panic(expected = "...")] attribute already documents; they can be dropped per the "paraphrasing the code" rule in engineering standards.
crates/contract/src/lib.rs:107-113 — doc comment on MINIMUM_NODE_MANAGEMENT_DEPOSIT hard-codes the list of methods it protects (register_backup_service, start_node_migration, update_participant_url). This list will drift if the constant is reused for another method later; consider phrasing it as "operator-authenticated node-management methods" without an enumeration, or grep-friendly TODO to keep them in sync.
docs/node-migration-guide.md — the guide's inline snippet for update_participant_url (referenced at line 9) isn't shown in the file, but any operator playbook or runbook that includes a full near contract call-function example for update_participant_url should also be updated from 0 NEAR to 1 yoctoNEAR. Worth a quick grep beyond docs/ (internal-ops/, READMEs of crates/backup-cli, docs/migration-service.md) — I only see node-migration-guide.md and backup-cli/src/backup.rs touched in this PR.

✅ Approved

claude · 2026-07-21T07:27:09Z

PR title type suggestion: This PR modifies the behavior of existing contract functions (adding deposit requirements), which is a functional change rather than routine maintenance. The type prefix should probably be fix: (if this addresses a missing security measure) or feat: (if adding new functionality) instead of chore:.

Suggested title: fix: require deposits for a few migration functions

haiyuechen-nearone

LGTM

pbeza · 2026-07-21T10:33:38Z

+        // Given
+        let running_state = gen_running_state(NUM_DOMAINS);
+        let account_id = running_state.parameters.participants().participants()[0]
+            .0
+            .clone();
+        let mut contract =
+            MpcContract::new_from_protocol_state(ProtocolContractState::Running(running_state));
+        let mut test_env = Environment::new(None, Some(account_id), None);
+        test_env.set_deposit(NearToken::from_yoctonear(0));
+        let backup_service_info = BackupServiceInfo {
+            public_key: bogus_ed25519_public_key(),
+        };


There’s a bit of copy-paste between these two tests. Perhaps it’s worth extracting the boilerplate, if possible?

For these tests we usually have a similar structure, that extracting might hide. Leaving as is for now, although we could consider changing it if the boilerplate keeps growing

Copilot

Pull request overview

Adds a 1 yoctoNEAR deposit requirement to selected operator-initiated node-migration / node-management contract methods to prevent calls from restricted keys, and updates tooling/tests/docs accordingly.

Changes:

Mark register_backup_service, start_node_migration, and update_participant_url as #[payable] and enforce a minimum attached deposit via require_deposit.
Update E2E harness + backup-cli output + migration guide examples to attach 1 yoctoNEAR.
Extend contract test utilities and add/adjust tests to cover “no deposit attached” rejections.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
docs/node-migration-guide.md	Updates example `near` CLI invocations to attach `1 yoctoNEAR` for migration-related calls.
crates/e2e-tests/src/cluster.rs	Switches operator calls to use a new “call with deposit” helper and attaches `1 yoctoNEAR`.
crates/e2e-tests/src/blockchain.rs	Introduces `call_from_deposit` convenience wrapper (JSON args + default gas).
crates/contract/tests/snapshots/abi__abi_has_not_changed.snap	ABI snapshot updated to reflect payable modifiers + updated method docs.
crates/contract/tests/inprocess/update_participant_url.rs	Updates inprocess tests to attach deposit; adds a “reject without deposit” test.
crates/contract/tests/inprocess/common.rs	Adds `participant_context_with_deposit` VM context helper for payable methods.
crates/contract/src/tee/test_utils.rs	Extends `Environment` to track/apply attached deposit in VM context.
crates/contract/src/lib.rs	Adds `MINIMUM_NODE_MANAGEMENT_DEPOSIT`, makes methods payable, enforces deposit, and updates tests.
crates/backup-cli/src/backup.rs	Updates printed registration command to attach `1 yoctoNEAR`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…-is-required-for-migration-functions

claude · 2026-07-21T11:26:23Z

PR title type suggestion: This PR modifies contract behavior by adding deposit requirements to migration functions, which changes the contract's interface (ABI snapshot updated). Since source code changes add new requirements rather than routine maintenance, the type prefix should probably be feat: instead of chore:.

Suggested title: feat: require deposits for a few migration functions

claude · 2026-07-21T11:27:35Z

PR title type suggestion: This PR adds new requirements (deposits) to migration functions in the contract, which is a behavioral change rather than routine maintenance. The type prefix should probably be feat: instead of chore:.

Suggested title: feat: require deposits for a few migration functions

claude · 2026-07-21T12:28:06Z

PR title type suggestion: This PR adds a new functional requirement (deposits) to migration functions, which is a feature/capability addition rather than routine maintenance. The type should probably be feat: instead of chore:.

Suggested title: feat: require deposits for migration functions

claude · 2026-07-21T12:35:58Z

PR title type suggestion: This PR adds deposit requirements to three user-facing contract functions, which is a behavioral change to the contract's public API. Consider using feat: instead of chore: to better reflect that this is a new user-facing requirement.

Suggested title: feat: require deposits for migration functions

SimonRastikian · 2026-07-21T12:42:57Z

+/// Minimum deposit required for the operator-authenticated node-management methods
+/// (`register_backup_service`, `start_node_migration`, `update_participant_url`).
+///
+/// A non-zero deposit forces the call to be signed by a full-access key: the node's own key
+/// is registered as a function-call access key, which cannot attach a deposit, so a leaked
+/// node key cannot invoke these methods.


Nit: Could you compact this verbose comment?

It is very non-trivial why we require this deposits, so being a bit verbose here will probably help future devs

…-is-required-for-migration-functions

claude · 2026-07-22T14:09:59Z

PR title type suggestion: This PR modifies contract source code to add new behavior (deposit requirements for migration functions), so the type prefix should probably be feat: or fix: instead of chore:. Suggested title: feat: require deposits for a few migration functions (if adding the requirement as a new feature) or fix: require deposits for a few migration functions (if fixing missing validation).

haiyuechen-nearone

🚢

Resolve modify/delete on test-migration.sh by keeping the delete (the script moved to mpc-private). #3874's deposit change to that file is ported to the mpc-private copy (mpc-private#495).

gilcu3 linked an issue Jul 21, 2026 that may be closed by this pull request

check if deposit is required for migration functions #1163

Closed

gilcu3 force-pushed the 1163-check-if-deposit-is-required-for-migration-functions branch 2 times, most recently from b7ad059 to e207ebc Compare July 21, 2026 07:20

gilcu3 marked this pull request as ready for review July 21, 2026 07:20

chore: require deposits for a few migration functions

b456de8

gilcu3 force-pushed the 1163-check-if-deposit-is-required-for-migration-functions branch from e207ebc to b456de8 Compare July 21, 2026 07:25

haiyuechen-nearone previously approved these changes Jul 21, 2026

View reviewed changes

pbeza previously approved these changes Jul 21, 2026

View reviewed changes

pbeza requested a review from Copilot July 21, 2026 10:46

Copilot started reviewing on behalf of pbeza July 21, 2026 10:46 View session

Copilot AI reviewed Jul 21, 2026

View reviewed changes

Comment thread crates/contract/src/lib.rs

Comment thread crates/contract/src/lib.rs Outdated

Comment thread crates/contract/src/lib.rs Outdated

Comment thread crates/contract/src/lib.rs Outdated

Comment thread crates/contract/src/lib.rs

fix: address reviews

580c594

gilcu3 dismissed stale reviews from pbeza and haiyuechen-nearone via 580c594 July 21, 2026 11:25

Merge remote-tracking branch 'origin/main' into 1163-check-if-deposit…

ae98d5d

…-is-required-for-migration-functions

pbeza reviewed Jul 21, 2026

View reviewed changes

Comment thread crates/contract/src/lib.rs Outdated

pbeza reviewed Jul 21, 2026

View reviewed changes

Comment thread crates/contract/tests/inprocess/common.rs Outdated

pbeza previously approved these changes Jul 21, 2026

View reviewed changes

fix: address nits

63da8aa

gilcu3 dismissed pbeza’s stale review via 63da8aa July 21, 2026 12:26

fix: forgot this one

0922ef0

SimonRastikian previously approved these changes Jul 21, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into 1163-check-if-deposit…

efb7683

…-is-required-for-migration-functions

fix: address nits

b8be41e

gilcu3 dismissed SimonRastikian’s stale review via b8be41e July 22, 2026 14:08

gilcu3 changed the title ~~chore: require deposits for a few migration functions~~ feat: require deposits for a few migration functions Jul 22, 2026

pbeza approved these changes Jul 22, 2026

View reviewed changes

gilcu3 enabled auto-merge July 22, 2026 14:20

haiyuechen-nearone approved these changes Jul 22, 2026

View reviewed changes

gilcu3 added this pull request to the merge queue Jul 22, 2026

Merged via the queue into main with commit 86a999d Jul 22, 2026
29 of 30 checks passed

gilcu3 deleted the 1163-check-if-deposit-is-required-for-migration-functions branch July 22, 2026 14:33

Uh oh!

Conversation

gilcu3 commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Jul 21, 2026

Uh oh!

claude Bot commented Jul 21, 2026

Pull request overview

Reviewed changes

Findings

Uh oh!

claude Bot commented Jul 21, 2026

Uh oh!

haiyuechen-nearone left a comment

Choose a reason for hiding this comment

Uh oh!

pbeza Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

gilcu3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 21, 2026

Uh oh!

claude Bot commented Jul 21, 2026

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 21, 2026

Uh oh!

claude Bot commented Jul 21, 2026

Uh oh!

Uh oh!

SimonRastikian Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

gilcu3 Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot commented Jul 22, 2026

Uh oh!

haiyuechen-nearone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gilcu3 commented Jul 21, 2026 •

edited

Loading