Skip to content

feat(interface): typed vote and admin methods on MpcContractHandle - #3922

Merged
kevindeforth merged 5 commits into
mainfrom
kd/typed-vote-calls
Aug 10, 2026
Merged

feat(interface): typed vote and admin methods on MpcContractHandle#3922
kevindeforth merged 5 commits into
mainfrom
kd/typed-vote-calls

Conversation

@kevindeforth

@kevindeforth kevindeforth commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

resolves #3921, part of #3693

@kevindeforth
kevindeforth force-pushed the kd/typed-vote-calls branch from d100b5c to a5e813f Compare July 24, 2026 14:05
Base automatically changed from kd/typed-contract-updates to main July 24, 2026 20:20
@kevindeforth
kevindeforth force-pushed the kd/typed-vote-calls branch from e02ffa0 to 983363a Compare July 24, 2026 20:36
@kevindeforth
kevindeforth force-pushed the kd/typed-vote-calls branch from ff6c2c3 to afcab90 Compare July 24, 2026 20:47
Comment on lines +308 to +315
impl<E> From<PayloadBytesError> for MpcContractHandleError<E> {
fn from(value: PayloadBytesError) -> Self {
match value {
PayloadBytesError::Serialize(err) => MpcContractHandleError::Serialize(err),
PayloadBytesError::Overflow => MpcContractHandleError::Deposit(DepositOverflowError),
}
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🙏

@kevindeforth
kevindeforth marked this pull request as ready for review July 24, 2026 20:59
@claude

This comment was marked as resolved.

@kevindeforth

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Jul 24, 2026

Copy link
Copy Markdown

Pull request overview

Follow-up review after the prior blocking finding on update_participant_url was addressed. The typed handle now correctly attaches MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR on update_participant_url / register_backup_service / start_node_migration, the catalog snapshot has been regenerated (all three peer methods now show 1 yoctoNEAR), and the From<PayloadBytesError> impl replaces the inline match in propose_update.

Changes: unchanged from the earlier summary — the delta since is limited to the deposit fix on update_participant_url and the corresponding snapshot update.

Findings

Blocking: none — the prior update_participant_url deposit regression is resolved (see crates/near-mpc-contract-interface/src/client.rs:187-198 and .../catalog.snap:66-69, now 1 yoctoNEAR).

Non-blocking (nits, follow-ups):

  • crates/near-mpc-contract-interface/src/deposits.rs:14MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR: u128 = 1 silently duplicates the contract's MINIMUM_NODE_MANAGEMENT_DEPOSIT = NearToken::from_yoctonear(1) at crates/contract/src/lib.rs:118. There's no test tying the two together — if a future change bumps the contract-side minimum, the interface would keep sending the old value and every #[payable] node-management call would revert at require_deposit(...). Compare STORAGE_BYTE_COST_YOCTONEAR__should_match_env_storage_byte_cost at .../deposits.rs:49, which does exactly this kind of drift-check. Consider either a small compile-time or runtime assert in the interface crate's tests, or a short doc comment on the constant noting the pairing.

  • crates/near-mpc-contract-interface/src/client.rs:309-315 — the new From<PayloadBytesError> for MpcContractHandleError<E> maps PayloadBytesError::Overflow (payload exceeds u128::MAX bytes) to MpcContractHandleError::Deposit(DepositOverflowError) whose message is "the required deposit exceeds u128::MAX yoctoNEAR". The two overflows are correlated in propose_update (huge payload ⇒ huge deposit) but not semantically the same, and the resulting error message would be misleading if it ever fired via a different call path. This isn't a regression (the previous inline match did the same thing), but now that this conversion is a reusable From impl it's worth either giving PayloadBytesError::Overflow its own MpcContractHandleError::PayloadTooLarge variant, or refining the DepositOverflowError message to cover both origins.

  • crates/near-mpc-contract-interface/src/client.rs:45VOTE_FOREIGN_CHAIN_GAS: NearGas = NearGas::from_tgas(30) is now a pub const on the interface (previously an e2e-only constant). Unlike SIGN_GAS / CKD_PV_GAS right next to it, there's no comment justifying the 30 Tgas figure. A one-liner noting that this covers the borsh-decode + threshold vote apply path would keep it consistent with its neighbors.

  • Follow-up from prior review still open: the suggested contract-side test update_participant_url__should_reject_when_no_deposit_attached alongside crates/contract/src/lib.rs:5080 / :5098 is not in this PR. Not blocking here, but worth tracking — this is exactly the regression that slipped through the earlier revision of this PR.

⚠️ Issues found (non-blocking)

@netrome
netrome force-pushed the kd/typed-vote-calls branch from 804ff3b to 5074278 Compare August 7, 2026 12:46
netrome
netrome previously approved these changes Aug 7, 2026

@netrome netrome left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice stuff! I took the liberty to rebase the PR, but it already got a new conflict (with my other PR I believe). Will update and re-approve.

Comment on lines +41 to +45
contract: mpc.near
method: vote_add_domains
gas: 300.0 Tgas
deposit: 0 NEAR
args: {"domains":[{"id":0,"protocol":"CaitSith","reconstruction_threshold":2,"purpose":"Sign"}]}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are really nice! 🤩

Comment on lines +308 to +315
impl<E> From<PayloadBytesError> for MpcContractHandleError<E> {
fn from(value: PayloadBytesError) -> Self {
match value {
PayloadBytesError::Serialize(err) => MpcContractHandleError::Serialize(err),
PayloadBytesError::Overflow => MpcContractHandleError::Deposit(DepositOverflowError),
}
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🙏

@netrome

netrome commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

crates/near-mpc-contract-interface/src/deposits.rs:14 — MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR: u128 = 1 silently duplicates the contract's MINIMUM_NODE_MANAGEMENT_DEPOSIT = NearToken::from_yoctonear(1) at crates/contract/src/lib.rs:118. There's no test tying the two together — if a future change bumps the contract-side minimum, the interface would keep sending the old value and every #[payable] node-management call would revert at require_deposit(...). Compare STORAGE_BYTE_COST_YOCTONEAR__should_match_env_storage_byte_cost at .../deposits.rs:49, which does exactly this kind of drift-check. Consider either a small compile-time or runtime assert in the interface crate's tests, or a short doc comment on the constant noting the pairing.

Addressed in 86623a9

kevindeforth and others added 5 commits August 7, 2026 21:27
…ant_url

The contract requires MINIMUM_NODE_MANAGEMENT_DEPOSIT on
update_participant_url like on register_backup_service and
start_node_migration; the handle attached nothing, failing the
update_participant_url e2e test.
@netrome
netrome force-pushed the kd/typed-vote-calls branch from 86623a9 to 8a904cd Compare August 7, 2026 19:28

@netrome netrome left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took the liberty to rebase and resolve another conflict. Should be good to go now.

@pbeza
pbeza requested a balanced review from Copilot August 10, 2026 10:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds nine typed governance/admin methods to MpcContractHandle and migrates e2e callers away from hand-built wire payloads.

Changes:

  • Adds typed arguments, gas, and deposit handling.
  • Migrates governance, migration, and foreign-chain e2e calls.
  • Extends wire-format snapshot coverage.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/near-mpc-contract-interface/src/client.rs Adds typed handle methods.
crates/near-mpc-contract-interface/src/call_args.rs Adds typed JSON argument DTOs.
crates/near-mpc-contract-interface/src/deposits.rs Centralizes management deposit.
crates/near-mpc-contract-interface/src/snapshots/near_mpc_contract_interface__client__tests__mpc_contract_handle__should_match_the_wire_format_catalog.snap Records new wire formats.
crates/near-contract-transport/src/types.rs Adds function-call constructors.
crates/contract/src/lib.rs Reuses interface deposit constant.
crates/e2e-tests/src/blockchain.rs Removes superseded raw-call helpers.
crates/e2e-tests/src/cluster.rs Routes targeted calls through the handle.
crates/e2e-tests/src/mpc_node.rs Returns typed signer public keys.
crates/e2e-tests/tests/web_endpoints.rs Adapts public-key formatting.
crates/e2e-tests/tests/migration_service.rs Uses typed migration DTOs.
crates/e2e-tests/tests/migration_endpoint.rs Uses typed migration DTOs.
crates/e2e-tests/README.md Updates e2e contract-call documentation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +128 to +129
`NearKitCaller`), `call_from_with_deposit` (untyped escape hatch for
`prepay_attestation_storage`, which has no typed method yet), `view`, and
Comment on lines +126 to +127
client. It exposes `call`/`call_final` (from the contract account, used only
for `init`), `handle_for` (a typed `MpcContractHandle` calling as a given
@pbeza

pbeza commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

@claude review

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Pull request overview

Moves the e2e suite's vote/admin contract calls off the stringly-typed DeployedContract::call_from* helpers and onto MpcContractHandle, adding nine typed methods (vote_add_domains, vote_new_parameters, vote_cancel_keygen, vote_cancel_resharing, update_participant_url, register_backup_service, start_node_migration, register_foreign_chain_support, vote_update_foreign_chain_providers) each backed by a dedicated *Args struct. Method name, args encoding, gas, and deposit now live in one place per method, and the wire-format snapshot catalogs all of them. The untyped call helpers and their gas/deposit constants are deleted, and crates/contract now derives MINIMUM_NODE_MANAGEMENT_DEPOSIT from the interface crate's MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR instead of duplicating the literal.

Changes:

  • Nine typed methods on MpcContractHandle plus a private call helper that collapses the repeated call_contract(...).map_err(Call) boilerplate; From<PayloadBytesError> replaces the inline match in propose_update.
  • FunctionCallArgs::new / no_deposit constructors; MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR and VOTE_FOREIGN_CHAIN_GAS constants.
  • Removed call_from, call_from_deposit, call_from_borsh_with_deposit from DeployedContract and call_from_all_nodes_concurrently / NODE_MANAGEMENT_DEPOSIT / VOTE_FOREIGN_CHAIN_GAS from cluster.rs; every migrated site now goes through handle_for(...).
  • e2e tests build typed BackupServiceInfo / DestinationNodeInfo / ParticipantInfo instead of serde_json::Value; near_signer_public_key_str() becomes near_signer_public_key() -> Ed25519PublicKey.
  • Catalog test and snapshot extended from 9 to 18 recorded calls.

Reviewed changes

Per-file summary
File Description
crates/near-mpc-contract-interface/src/client.rs Nine typed vote/admin methods, private call helper, From<PayloadBytesError> impl, VOTE_FOREIGN_CHAIN_GAS, expanded catalog test.
crates/near-mpc-contract-interface/src/call_args.rs Seven new JSON *Args DTOs for the added methods.
crates/near-mpc-contract-interface/src/deposits.rs Adds MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR = 1.
crates/near-mpc-contract-interface/src/snapshots/...catalog.snap Records the nine new wire formats (gas, deposit, encoded args).
crates/near-contract-transport/src/types.rs FunctionCallArgs::new / no_deposit constructors.
crates/contract/src/lib.rs MINIMUM_NODE_MANAGEMENT_DEPOSIT now derives from the interface constant.
crates/e2e-tests/src/blockchain.rs Drops the three superseded untyped call helpers.
crates/e2e-tests/src/cluster.rs Routes vote/admin calls through handle_for(...); vote_resharing takes a typed EpochId + proposal; drops the local gas/deposit constants.
crates/e2e-tests/src/mpc_node.rs near_signer_public_key_str becomes near_signer_public_key.
crates/e2e-tests/tests/migration_endpoint.rs, migration_service.rs Typed migration DTOs instead of json!.
crates/e2e-tests/tests/web_endpoints.rs Formats the signer key at the call site.
crates/e2e-tests/README.md Describes the typed-handle-first call surface.

Findings

I checked the migrated call sites against the contract: register_backup_service, start_node_migration and update_participant_url are the only #[payable] methods among the nine (crates/contract/src/lib.rs:2774, :2816, :2851) and are exactly the three the handle attaches MINIMUM_NODE_MANAGEMENT_DEPOSIT_YOCTONEAR to; register_foreign_chain_support (:1135) and vote_update_foreign_chain_providers (:1748) are non-payable and correctly get a zero deposit. Gas, args shape and signer key (node_keys vs operator_keys) are preserved at every rewritten site, and operator_client_for(node_index) is equivalent to the client construction it replaced in register_foreign_chain_support. No blocking issues.

Non-blocking (follow-ups, nits):

  • crates/devnet/src/mpc.rs:682 and :918 — devnet still defines its own private VoteAddDomainsArgs { domains } and VoteNewParametersArgs { prospective_epoch_id, proposal }, now field-identical duplicates of the public ones this PR adds in call_args.rs. The workspace therefore ends up with two definitions of those two wire formats, which cuts against the "single source of each method's wire format" the README now claims. Deleting devnet's copies and importing from near_mpc_contract_interface::call_args would be a small mechanical follow-up (devnet is outside feat: typed vote and admin methods on MpcContractHandle #3921's scope, so not blocking here).

  • crates/near-mpc-contract-interface/src/client.rs:189, :203, :217 — the three deposit-bearing methods carry no doc note that they cannot be issued with the node's function-call access key. That matters because the same handle is driven by node-side code holding exactly such a key (crates/tee-context/src/lib.rs:98), so a future caller reaching for update_participant_url from there gets a runtime failure rather than a compile error. submit_participant_info already documents the mirror-image constraint inline ("The node's function-call key cannot attach a deposit"); one line of the same on these three would make the requirement discoverable from the consumer side, as CLAUDE.md asks for public APIs.

  • crates/near-mpc-contract-interface/README.md:20 — "While this crate only contains plain data types (or DTOs) at the time of writing, long term we may want to extend this to also include helper functions to construct requests" is stale (it predates client.rs), and this PR makes it noticeably more so by adding nine request-constructing helpers. Since that README is the crate-level rustdoc (#![doc = include_str!("../README.md")]), a one-line refresh of the Vision section would keep the crate docs honest.

✅ Approved

@kevindeforth
kevindeforth added this pull request to the merge queue Aug 10, 2026
Merged via the queue into main with commit 0a31d96 Aug 10, 2026
16 checks passed
@kevindeforth
kevindeforth deleted the kd/typed-vote-calls branch August 10, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(e2e): retire the raw contract-call helpers feat: typed vote and admin methods on MpcContractHandle

4 participants