MCMS Refactor: Replace Solidity-ABI-over-XDR leaf encoding with domain-separated byte layout - #171
Draft
JohnChangUK wants to merge 1 commit into
Draft
MCMS Refactor: Replace Solidity-ABI-over-XDR leaf encoding with domain-separated byte layout#171JohnChangUK wants to merge 1 commit into
JohnChangUK wants to merge 1 commit into
Conversation
…sioned, domain-separated byte layout, and reshape the operation/metadata types to describe the actual Soroban call.
There was a problem hiding this comment.
Pull request overview
Refactors MCMS-Stellar leaf hashing away from “Solidity ABI over XDR” into an explicit, versioned, domain-separated byte layout, and updates on-chain types/bindings/tests to represent Soroban invocations directly (contract Address + function Symbol + argument XDR bytes).
Changes:
- Introduces
encoding.rswith domain-separated, versioned preimages forStellarRootMetadataandStellarOp(and removesabi_encoding.rs). - Reshapes MCMS operation/metadata types (network_id,
Addressfields,function/args_xdr,encoding_version,config_version) and enforces these inset_root/execute. - Adds a normative golden vector fixture and updates Rust tests plus Go bindings to match the new API/encoding.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| contracts/mcms/testdata/stellar_golden_vectors.json | Adds normative cross-language golden vectors for the new encoding and hashes. |
| contracts/mcms/test_snapshots/test/test_execute_reverted_call_does_not_consume_nonce.1.json | Removes an outdated snapshot tied to the pre-refactor behavior/format. |
| contracts/mcms/src/types.rs | Updates on-chain structs to use Address/Symbol, add encoding_version and config_version, and split function vs args. |
| contracts/mcms/src/test.rs | Reworks tests to build new StellarOp/metadata shapes, adds new revert/abort/config-version behaviors, and aligns with new encoding. |
| contracts/mcms/src/lib.rs | Switches contract logic to new encoding, adds config versioning + instance label storage, updates validation and emitted events. |
| contracts/mcms/src/events.rs | Updates emitted event payloads (config_version, target/function/args_hash) and removes min-secs-per-ledger event. |
| contracts/mcms/src/error.rs | Adds new error variants for version/config/address/XDR validation and distinguishes CallAborted. |
| contracts/mcms/src/encoding.rs | New encoding + hashing module with domain-separated, length-prefixed preimages and golden-vector tests. |
| contracts/mcms/src/constants.rs | Adds ENCODING_VERSION and removes dynamic-validity/min-secs-per-ledger constants in favor of a fixed policy horizon. |
| contracts/mcms/src/abi_encoding.rs | Deleted legacy Solidity ABI encoding implementation. |
| contracts/mcms/Cargo.toml | Moves stellar-strkey to dev-dependencies for encoding test support. |
| contracts/common/interfaces/src/mcms.rs | Updates interface types and contract events to match new structs/API and new error codes. |
| bindings/contracts/mcms/types.go | Updates Go struct/SCVal conversions for new op/metadata layouts and error codes. |
| bindings/contracts/mcms/client.go | Updates Go client initialize signature and adds getters for config version / instance label; removes min-secs-per-ledger calls and event helpers. |
Files not reviewed (2)
- bindings/contracts/mcms/client.go: Generated file
- bindings/contracts/mcms/types.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+312
to
318
| // A callee-returned contract error arrives as `Err(Ok(InvokeError::Contract(_)))`; | ||
| // the outer `Ok(Err(_))` arm only covers return-value conversion failures. | ||
| match env.try_invoke_contract::<Val, InvokeError>(&op.target, &op.function, args) { | ||
| Ok(Ok(_)) => {} | ||
| Ok(Err(_)) | Err(_) => return Err(McmsError::CallReverted), | ||
| Err(Ok(InvokeError::Contract(_))) => return Err(McmsError::CallReverted), | ||
| Ok(Err(_)) | Err(_) => return Err(McmsError::CallAborted), | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Not true, these tests prove the opposite
test_execute_reverted_call_does_not_consume_nonce_and_can_retrysets the callee (ExecFailureMock::maybe_fail) to returnErr(ExecMockError::Rejected)— a typed contract error — and assertsErr(Ok(McmsError::CallReverted)). ✅ passes.test_execute_aborted_call_is_distinct_and_does_not_consume_noncecallsExecFailureMock::abort()(a panic!/host trap) and assertsErr(Ok(McmsError::CallAborted)). ✅ passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the Solidity-ABI-over-XDR leaf encoding with an explicit, versioned, domain-separated byte layout, and reshapes the MCMS operation/metadata types to describe the actual Soroban call.
Key changes
abi_encoding.rsdeleted →encoding.rsadded: length-prefixed, domain-separated preimages (metadata 125 bytes, operation 184 in the golden vector);encoding_versionchecked before hashing;contract_id()rejects account addresses.stellar_golden_vectors.jsonadded — normative cross-language fixture, asserted from Rust (and Go once the deployment adapter is fixed).StellarOp:chain_id→network_id,multisig/tobecomeAddress,datasplits intofunction: Symbol+args_xdr: Bytes,valueremoved,encoding_versionadded.StellarRootMetadata: same address/naming changes, plusconfig_versionandencoding_version.