fix(ICP_Rosetta): DEFI-1800: Use a deterministic memo of 0 instead of a random one#10537
Draft
mbjorkqvist wants to merge 7 commits into
Draft
fix(ICP_Rosetta): DEFI-1800: Use a deterministic memo of 0 instead of a random one#10537mbjorkqvist wants to merge 7 commits into
mbjorkqvist wants to merge 7 commits into
Conversation
…oads Add a unit test that reconstructs the same transfer three times without a caller-specified memo and asserts that at least two of the resulting memos differ. This captures the current behavior, where construction_payloads substitutes a fresh random memo whenever the caller omits one (DEFI-1800). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the caller omits the memo in construction_payloads, substitute Memo(0) (the ledger's "no memo" value) instead of a random memo. The random memo was made part of the transaction hash, so reconstructing the same transfer produced a different hash on every call and defeated the ledger's created_at_time based deduplication, allowing a retried transfer to be applied twice. Using a deterministic memo lets the ledger dedup catch such retries. This also drops the now-unused rand import and removes the stale FIXME. Note: the randomness test added in the previous commit now fails on purpose; it is updated to assert the new behavior in the following commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update the memo test to match the new behavior: three subsequent reconstructions of the same transfer without a caller-specified memo must all produce a memo of 0, so that the ledger's created_at_time based deduplication catches a retried transfer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update the `memo` field doc-comment (no longer "a random number by default") and the stale test comment, and add a CHANGELOG entry for the change from a random to a deterministic memo of 0 in construction_payloads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a determinism bug in the ICP Rosetta Construction API: when callers omit memo in construction/payloads, the handler now uses a deterministic default (Memo(0)) instead of generating a random memo, preventing transaction-hash churn that can bypass created_at_time-based deduplication on retries.
Changes:
- Default memo handling in ICP Rosetta
construction/payloadstoMemo(0)when unspecified (removes random memo generation). - Update metadata documentation to reflect the new default memo behavior.
- Add/adjust tests and update the ICP Rosetta changelog to capture the behavioral change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rs/rosetta-api/icp/src/request_handler/construction_payloads.rs | Replaces random memo fallback with deterministic Memo(0) during payload construction. |
| rs/rosetta-api/icp/src/request_handler/construction_parse.rs | Updates tests/fixtures to align with deterministic memo behavior and adds a targeted memo-default test. |
| rs/rosetta-api/icp/src/models.rs | Updates API metadata documentation comment to reflect the default memo value of 0. |
| rs/rosetta-api/icp/CHANGELOG.md | Documents the changed default memo behavior and its deduplication implications. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The random memo was the only user of the rand crate in ic-rosetta-api; remove it from Cargo.toml and the BUILD.bazel targets. rand_chacha (used by tests) is retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Strengthen the test to pin all time-based inputs and assert that three reconstructions of the same transfer yield byte-identical unsigned transactions (hence the same transaction hash), in addition to a memo of 0. The comment now matches what is actually verified. - Extract the shared handler/key/operations setup into setup_transfer_test and use it from test_payloads_parse_identity too, removing the duplicated boilerplate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mbjorkqvist
changed the title
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.
Purpose
When a caller does not specify a memo in ICP Rosetta's
construction/payloads, Rosetta currently substitutes a random memo. Because the memo is part of the ledger transaction hash, reconstructing the same transfer (same operations, same pinnedcreated_at_time, no memo) produces a different hash every time. This defeats the ledger'screated_at_timebased deduplication, so a retried transfer can be applied twice and debit the sender twice.This contradicts the documented behavior (the memo should be left blank when omitted).
This PR makes the fallback deterministic: when no memo is specified, Rosetta now uses
Memo(0). Reconstructing the same transfer then yields the same transaction hash, so the ledger's deduplication catches retries. ICRC Rosetta is unaffected (it already leaves the memo absent).Memo(0)is used rather than truly omitting the memo because the native ICP ledgermemois a mandatoryu64field and cannot be omitted at the protocol level.Behavioral change
Transfers constructed without an explicit memo now use
memo = 0instead of a random value. As a result, two otherwise-identical transfers that share a pinnedcreated_at_timeand specify no memo will collapse to a single on-chain transfer (the second is rejected as a duplicate). To send multiple otherwise-identical transfers, supply a distinctmemoorcreated_at_time. With the defaultcreated_at_time(current time), separate calls differ naturally and are unaffected.