[yashvanthbl137-crypto]

Summary

Cosmos-family withdrawals to memo-required destinations fail verification with Tx outputs does not match with expected txParams recipients.

CosmosCoin.verifyTransaction compares txParams.recipients[].address as raw strings against explained outputs. Cosmos destinations represent their memo in more than one way across clients: the enterprise UI and wallet APIs fold it into the address as cosmos1...?memoId=X, while the retail UI sends a plain address plus a tx-level memo param. explainTransaction() reports the plain on-chain address plus a tx-level memo field — so the comparison always fails for combined-form recipients, even when the transaction was built correctly. Withdrawals from ATOM wallets to memo-required destinations were effectively impossible.

Fix

Two parts:

sdk-coin-atom — override verifyTransaction (Atom/Tatom) and normalize both sides to {address, amount, memo?} before comparing. A recipient's memo may be declared via the ?memoId= address suffix, a recipient.memo field, or the tx-level txParams.memo.value; all declared sources must agree, and empty strings are treated as absent. A declared memo must match the transaction memo exactly; when no memo is declared anywhere, the transaction must not carry one either — a memo the user never approved is a prebuild-integrity failure.

sdk-core (resolveEffectiveTxParams) — propagate the persisted intent.memo into effective txParams when the caller supplies none. Signing paths that carry no txParams (pendingApproval.approve() -> recreateTxRequest() -> signTxRequest()) otherwise reached the coin verifier with a memo-less txParams while the prebuilt transaction carried the memo approved at send time, which would have made the strict check reject legitimate re-signs.

Ports the per-coin precedent from sdk-coin-tia (c2725f263b, COINS-1760) and sdk-coin-zeta (51d997c5d7, CSHLD-1780); also declares the lodash dependency those modules already use.

Testing

• New unit tests in sdk-coin-atom: memo-suffixed recipient verifies; wrong/missing memo rejected; tx-level and recipient-level memo accepted; conflicting memo declarations rejected; empty tx-level memo with a memo-less tx verifies; a memo transaction re-signed from a persisted intent verifies through resolveEffectiveTxParams; a re-sign with a mismatched intent memo is rejected; malformed memo suffix rejected with InvalidAddressError.
• New unit tests in sdk-core: intent.memo propagated when txParams carries none; caller-supplied memo not overwritten; absent/empty intent memo stays undefined.
• Full sdk-coin-atom suite: 90 passing; sdk-core recipientUtils suite: 44 passing.
• Reproduced the original failure end-to-end on testnet (tatom UI withdrawal) and verified the exact error before this fix.

Ticket: COINS-2285

[linear-code]

COINS-2285

[github-actions]

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[yashvanthbl137-crypto]

@claude review

[github-actions]

Claude finished @yashvanthbl137-crypto's task in 25s —— View job

---

Review of #10113: normalize memo-suffixed recipients in verifyTransaction

The fix is correct for the reported bug, and I found no blocking issues. I read the diff and compared it with CosmosCoin.verifyTransaction and getAddressDetails in abstract-cosmos. I didn't run the tests.

• Read the diff and the base-class implementation
• Post review

What looks good

• Both sides are normalized to {address, amount, memo?}, and the expected memo comes only from txParams. A malicious or buggy prebuild therefore can't widen what is accepted.
• Conflicting memo declarations (?memoId=, recipient.memo, txParams.memo.value) are rejected rather than resolved by precedence. Empty strings count as absent.
• The lodash and @types/lodash versions match sdk-coin-tia and sdk-coin-zeta.

Points to consider

1. Behaviour change for recipients with no declared memo (modules/sdk-coin-atom/src/atom.ts, the _.isEqual comparison). The base class picked only address and amount, so it ignored the tx memo. This override also compares memo.

   • A tx that carries a memo while the recipient declares none is now rejected.
   • This is stricter than before and matches the PR description.
   • Flows where the memo lives only in the prebuild would start failing, for example consolidation or sweep builders, or callers that set the memo outside txParams. Please confirm none exist for ATOM.
   • The tests cover "recipient missing memo when tx carries one", so the behaviour is deliberate. I'd still mention it in the release notes.

2. Duplicated logic. The override copies the whole base method (builder, explain, total-amount check) to change a few lines.

   • This follows the tia/zeta precedent, so it's consistent.
   • It will drift from CosmosCoin if the base changes. This is the third copy of the same code.
   • A follow-up could add a protected hook in abstract-cosmos (for example normalizeRecipients) and drop the copies. It's not needed for this PR.

3. getAddressDetails can throw InvalidAddressError. It throws for malformed ?foo=bar suffixes. That is reasonable here, since a malformed destination should fail, but the error text differs from the usual "Tx outputs does not match…". It's worth a test asserting that a malformed suffix is rejected, if one doesn't exist yet.

4. Minor: error message for conflicting memos. It reuses the generic "Tx outputs does not match with expected txParams recipients". That's fine for compatibility. A more specific message would help debugging, but it could break callers that match on the string.

5. Minor: package.json ordering. @types/lodash is added after axios and not alphabetically. This is cosmetic, and tia and zeta do the same.

6. Node 27-nightly CI warning. The compatibility check is experimental and non-blocking, and I don't see how this diff would cause it.

Verdict: Looks good to merge once point 1 is confirmed. I'd also like a test for point 3.
· branch yashvanthbl/coins-2285

[yashvanthbl137-crypto]

@claude re-review

[github-actions]

Claude finished @yashvanthbl137-crypto's task in 34s —— View job

---

Re-review of #10113

The blocking issue from my earlier inline review is fixed. I read the new code and the added tests, but I didn't run the test suites.

• Read the updated diff
• Check the re-sign path against the earlier finding
• Check for side effects of the sdk-core change

Earlier finding: resolved. resolveEffectiveTxParams (recipientUtils.ts:192-199) now copies a non-empty intent.memo into effectiveTxParams.memo when the caller supplies none. Re-signs that carry no txParams (pendingApproval.approve() → recreateTxRequest() → signTxRequest()) now give Atom.verifyTransaction the approved memo, so the strict "no declared memo means the tx must not carry one" check no longer rejects legitimate re-signs. The new tests cover both the matching and the mismatched intent memo (atom.ts tests around lines 236 and 266). They also cover the malformed ?memoId suffix, which was my third point last time.

Side effects of the sdk-core change.

• The propagation only fires when txParams.memo is unset and intent.memo is a non-empty string. A caller-supplied memo is never overwritten.
• resolveEffectiveTxParams is used by ecdsa.ts, ecdsaMPCv2.ts and eddsaMPCv2.ts. A grep for txParams.memo across module src finds no other coin reading it, so no other coin's verification should change.
• intent.memo is written from params.memo?.value (mpcUtils.ts:417 and :428), which is a string, so the typeof guard is correct. The synthesized type: 'text' matches the Memo shape.

Remaining minor points (none blocking)

1. Stricter behaviour for ATOM. A tx that carries a memo while txParams declares none now fails. Please confirm that no ATOM flow, such as sweep or consolidation, builds a memo outside txParams and the persisted intent. This is worth a release note.
2. Logic copied from the base class. This is the third copy, after tia and zeta. A protected hook in abstract-cosmos would remove the duplication in a follow-up.
3. Error text for conflicting memos. It reuses the generic "Tx outputs does not match…" message. A more specific message would help debugging, but it could break callers that match on the string.

Verdict: No blocking issues remain. Once point 1 is confirmed, this looks good to merge.
· branch yashvanthbl/coins-2285