Noble send: memo over 256 chars is signed then rejected at broadcast (memo too large) (#5435)

[Vaulty-bot]

Fixes #5435

Changes

• Chain.kt: new Chain.maxMemoCharacters extension property returning each Cosmos-SDK chain's max_memo_characters (512 for GaiaChain, 256 for Noble/Osmosis/Kujira/Dydx/Terra/…), null for chains with no enforceable ceiling.
• SendFormUiModel.kt: added memoError: UiText? field, an isMemoBlocking derived property (only blocks when hasMemo is true and defiType == null), wired isMemoBlocking into isContinueDisabled(), and added memoLengthErrorOrNull(chain, memo) which takes maxOf(codePointCount, UTF-8 byte size) so multi-byte memos that pass the character count but exceed the node's byte limit still error.
• SendFormGraph.kt: new scope.launch combining selectedToken with memoFieldState.textAsFlow() to recompute memoError per keystroke and on network switch (limit is per-chain).
• DefaultSendStrategy.kt: re-validates the user memo with memoLengthErrorOrNull before building the transaction, throwing InvalidTransactionDataException so a submit racing the form's validation can't start a keysign that fails at broadcast (Noble send: memo over 256 chars is signed then rejected at broadcast (memo too large) #5435).
• SendFormAmountSection.kt: memo field in FoldableAmountWidget now auto-expands via LaunchedEffect(state.memoError), can't be collapsed while erroring, and renders VsTextInputFieldInnerState.Error + footNote with the error text; new send_error_memo_too_long string added to values/ plus all 9 locale files.
• SendFormContinueGateTest.kt: 7 new tests covering blocking on plain sends, non-blocking without a memo field, the at-limit boundary, per-chain limits, chains with no ceiling, error format args, and the emoji byte-limit case. (Coins.kt/EvmCoinFinder.kt churn appears to be unrelated reformatting.)

Checklist

• Lint clean
• Build verified (S1)
• Self-reviewed against requirements + simplify (S3)
• No secrets committed
• Conventional commit messages

Summary by CodeRabbit

• New Features
  • Added memo length validation based on the selected network’s limit.
  • Overlong memos now show a localized error (reporting UTF-8 byte length and the maximum allowed).
  • Continue/submission are blocked when a memo is present and invalid (excluding DeFi memo flows).
  • Memo field automatically expands and switches to an error presentation while the issue is active.
• Bug Fixes
  • Added a final submission-time check to prevent overlong memos from proceeding.
• Tests
  • Expanded coverage for memo gating, per-chain limits, whitespace handling, and emoji byte-length behavior.
• Chores
  • Added the new send_error_memo_too_long string in supported languages.

[coderabbitai]

📝 Walkthrough

Walkthrough

The send flow now validates memo length against per-chain limits, updates errors as the token or memo changes, blocks continuation for invalid plain sends, displays localized field errors, and rechecks memo length before submission. Unrelated formatting changes preserve existing behavior.

Changes

Memo validation flow

Layer / File(s)	Summary
Chain limits and gating data/src/main/kotlin/.../Chain.kt, app/src/main/java/.../SendFormUiModel.kt, app/src/test/.../SendFormContinueGateTest.kt, app/src/main/res/values*/strings.xml	Per-chain memo ceilings and UTF-8 byte-length validation are added; invalid memos block applicable sends, with localized errors and coverage for chain limits, blank values, and emoji.
Live validation and error display app/src/main/java/.../SendFormGraph.kt, app/src/main/java/.../SendFormAmountSection.kt	Memo errors are recomputed from token and field changes, then shown in an expanded error-state memo field.
Submission-time validation app/src/main/java/.../DefaultSendStrategy.kt	Submission revalidates the normalized memo and rejects over-limit values before transaction construction.

Source formatting cleanup

Layer / File(s)	Summary
Data and lookup formatting data/src/main/kotlin/.../Coins.kt, data/src/main/kotlin/.../EvmCoinFinder.kt	Robinhood stock-token declarations and an EVM coin lookup predicate are reformatted without described behavior changes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant SendFormGraph
  participant SendFormUiModel
  participant SendFormAmountSection
  participant DefaultSendStrategy
  User->>SendFormGraph: enter memo or change token
  SendFormGraph->>SendFormUiModel: update memoError
  SendFormUiModel->>SendFormAmountSection: expose blocking error state
  SendFormAmountSection-->>User: show expanded memo error
  User->>DefaultSendStrategy: submit send
  DefaultSendStrategy->>DefaultSendStrategy: revalidate memo length
  DefaultSendStrategy-->>User: reject invalid transaction data

Loading

Possibly related PRs

• vultisig/vultisig-android#5247: Both changes modify memo processing in DefaultSendStrategy.submit().
• vultisig/vultisig-android#5368: Both changes modify send-form continuation gating and SendFormGraph state handling.

Suggested reviewers: aminsato

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The memo gate is implemented with UTF-8 byte counting, but the issue requires enforcing the chain's max_memo_characters rule by character count.	Switch validation and error messaging to the same per-chain character-count rule used by max_memo_characters, then keep the pre-sign block and tests aligned.
Out of Scope Changes check	⚠️ Warning	Coins.kt and EvmCoinFinder.kt contain unrelated formatting/refactor changes that are outside the memo-limit fix.	Remove those unrelated edits and keep the PR scoped to memo validation, UI gating, and supporting tests/resources.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the Noble send memo-limit fix and matches the main change in the PR.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent/5435-noble-send--memo-over-256-chars-is-signe

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@app/src/main/res/values/strings.xml`:
- Line 178: Update send_error_memo_too_long to describe the measured UTF-8 value
as bytes or with unit-neutral wording in all affected files:
app/src/main/res/values/strings.xml lines 178-178,
app/src/main/res/values-pt/strings.xml lines 198-198,
app/src/main/res/values-ru/strings.xml lines 105-105, and
app/src/main/res/values-zh-rCN/strings.xml lines 197-197. Keep the placeholders
and maximum-value meaning unchanged.

In `@data/src/main/kotlin/com/vultisig/wallet/data/models/Chain.kt`:
- Around line 563-572: Update the chain memo-size mapping in Chain’s relevant
size-limit function so Chain.ThorChain is handled separately with a 250-byte
limit instead of sharing 256 with the other chains. Add boundary coverage
verifying 250-byte memos are accepted and 251-byte memos are rejected for
THORChain.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d6dba67a-108b-48b5-a9d8-544796d37fa1

📥 Commits

Reviewing files that changed from the base of the PR and between 27fca34 and ae927e7.

📒 Files selected for processing (18)

• app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormGraph.kt
• app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormUiModel.kt
• app/src/main/java/com/vultisig/wallet/ui/models/send/submit/DefaultSendStrategy.kt
• app/src/main/java/com/vultisig/wallet/ui/screens/send/SendFormAmountSection.kt
• app/src/main/res/values-de/strings.xml
• app/src/main/res/values-es/strings.xml
• app/src/main/res/values-hr/strings.xml
• app/src/main/res/values-it/strings.xml
• app/src/main/res/values-ko/strings.xml
• app/src/main/res/values-nl/strings.xml
• app/src/main/res/values-pt/strings.xml
• app/src/main/res/values-ru/strings.xml
• app/src/main/res/values-zh-rCN/strings.xml
• app/src/main/res/values/strings.xml
• app/src/test/java/com/vultisig/wallet/ui/models/send/SendFormContinueGateTest.kt
• data/src/main/kotlin/com/vultisig/wallet/data/models/Chain.kt
• data/src/main/kotlin/com/vultisig/wallet/data/models/Coins.kt
• data/src/main/kotlin/com/vultisig/wallet/data/usecases/EvmCoinFinder.kt

[Vaulty-bot]

Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it.

[Vaulty-bot]

Review feedback addressed — 34d1a70

Inline comments (2/2 replied)

Comment	Status
Describe the measured unit accurately (values/strings.xml)	Fixed
Use THORChain’s 250-byte memo limit (Chain.kt)	Fixed

Measured unit. memoLengthErrorOrNull now measures UTF-8 bytes only. The previous maxOf(codePointCount, byteLength) was always equal to the byte length — UTF-8 encodes every code point in at least one byte — so the extra term added nothing but the ambiguity the comment flagged. All ten locales now say bytes rather than characters (values, de, es, hr, it, ko, nl, pt, ru, zh-rCN), with placeholders and limit semantics unchanged.

THORChain. Split out as Chain.ThorChain -> 250; MayaChain stays at 256 as the cited source only covers THORChain. Boundary test added at 250/251.

Pre-merge checks

Out of Scope Changes — addressed. Coins.kt (2122 lines) and EvmCoinFinder.kt are reverted to their main state. Both were touched only by the blanket ./gradlew ktfmtFormat in eed5f06 and neither is related to memo validation. Note that both files are unformatted on main, so ktfmtCheck flags them there too — that is pre-existing and not gated by CI, which runs assembleDebug testDebugUnitTest, lintDebug, and connected tests. Files this PR actually changes are ktfmt-clean.

Linked Issues — respectfully disagree with the suggested fix. The check asks that validation match a character limit and accept any memo at or under it regardless of byte size. That would reintroduce the bug: the Cosmos SDK compares len(memo), a Go string’s UTF-8 byte length, against max_memo_characters, so a memo of 200 emoji is 800 bytes to the node and is rejected at broadcast against a 256 ceiling. Byte counting is what makes the gate agree with the node. The parameter’s name is the misleading part, and Chain.maxMemoCharacters’ KDoc now says outright that it is enforced as a byte ceiling.

Docstring Coverage — no action. Every new public declaration already carries KDoc (Chain.maxMemoCharacters, isMemoBlocking, memoLengthErrorOrNull); the reported 0.00% does not match the diff. Test methods are self-describing by name and were left as-is per the no-unnecessary-comments guidance.

Verification

ktfmtCheck passes for every file this PR changes. Unit tests could not run locally — com.trustwallet:wallet-core:4.7.0 resolves from GitHub Packages and returns 401 without GITHUB_USER/GITHUB_TOKEN credentials, which this environment does not have — so SendFormContinueGateTest is verified by CI on this push rather than locally.

[Vaulty-bot]

Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it.

[Vaulty-bot]

Addressed the pre-merge check warnings. Commit: 0e2ae9b11.

✅ Fixed — Out of Scope Changes check

A massive unrelated Coins.kt reformat and a small EvmCoinFinder refactor were introduced outside the memo-limit scope.

Valid. Both files had been reflowed by a repo-wide ./gradlew ktfmtFormat run but carried no functional change for this PR, so Coins.kt and EvmCoinFinder.kt are reverted to main byte-for-byte. The Coins.kt churn was pure line-wrapping — git diff --ignore-all-space reduced its 2122 changed lines to 18, all of them ktfmt moving val STOCK_TOKENS = listOf( and five long logo = URLs onto their own lines.

The PR diff drops from 1253 insertions / 1062 deletions across 18 files to 187 insertions / 1 deletion across 16 files, now entirely the memo-limit fix. Reverting is safe for CI: .github/workflows/android.yml runs assembleDebug, testDebugUnitTest and lintDebug only — there is no ktfmtCheck gate, and these two files were already in this state on main.

❌ Declined — Linked Issues check

memo validation overcounts by UTF-8 bytes, so valid multibyte memos may be rejected. Match validation to the chain's character limit exactly, and allow any memo at or under the limit regardless of UTF-8 byte size.

Byte counting is the correct behaviour here, and this warning asks for the change that would reintroduce the bug. The Cosmos SDK's ValidateMemoDecorator compares len(memoTx.GetMemo()) against max_memo_characters, and len() on a Go string is its UTF-8 byte length, not a character count. So a 200-emoji memo genuinely is 800 for the node's purposes and genuinely is rejected at broadcast against Noble's 256 ceiling — after signing, which is exactly the failure issue #5435 describes.

Validating by character count instead would let those transactions pass the gate and burn a full multi-device keysign ceremony on a payload the chain can never accept. The param's name is the misleading part; the KDoc on Chain.maxMemoCharacters and on memoLengthErrorOrNull both state explicitly that it is enforced as a byte ceiling, and memo within the character limit but over the node's byte limit errors pins the behaviour. This is also the same conclusion reached on the strings.xml thread, where CodeRabbit's inline review asked for the opposite fix — that the strings say "bytes" rather than "characters" precisely because the measured unit is bytes. Both cannot be satisfied at once; the inline comment is the one that matches node behaviour, and it is what shipped.

❌ Declined — Docstring Coverage (0.00%)

Not reproducible against the diff. Every new public declaration this PR adds already carries KDoc: Chain.maxMemoCharacters, SendFormUiModel.isMemoBlocking, and memoLengthErrorOrNull each have a summary plus @param/@return where non-obvious, and the pre-existing isContinueDisabled KDoc was extended to document the new memo gate. The remaining changed files are strings.xml locale entries and a test class, neither of which takes KDoc. Reads like a miscomputed metric rather than a real gap.

Verification

./gradlew :app:testDebugUnitTest --tests "*SendFormContinueGateTest*" could not run in the agent sandbox — dependency resolution fails on com.trustwallet:wallet-core:4.7.0 with 401 Unauthorized, as the WalletCore GitHub Packages credentials (TRUSTWALLET_PAT) are not available there. CI has the secret, so the 11 gate tests run there. This commit only reverts two files to their main contents, so it cannot affect compilation or test outcomes.

Inline comments: 2 top-level threads on this PR, both already answered and marked confirmed-addressed in 34d1a70d1. This run's review surfaced no new inline comments — only the pre-merge checks above. Threads left open for the human reviewer to resolve.

[Vaulty-bot]

Addressed review feedback — 9897199

All three inline comments fixed; replies posted in each thread.

1. Whitespace-only memo permanently disabled Continue (id=3667442064)

memoLengthErrorOrNull now returns null for a blank memo. Fixed in the shared helper rather than at the SendFormGraph call site so the form gate and the DefaultSendStrategy re-check agree on what counts as "no memo" — the strategy already blanks a whitespace-only memo to null before signing, so the transaction carries no memo at all and its length can't be a reason to block.

2. ThorChain 250 was the wrong ceiling (id=3667442070)

Moved Chain.ThorChain into the 256 group. Confirmed the plain send path hardcodes isDeposit=false (DefaultSendStrategy.kt:248), so the memo rides in the tx envelope and is validated against max_memo_characters (256 on thorchain-1) — the 250-byte MsgDeposit cap doesn't apply and was wrongly blocking valid 251-256 byte memos. Added a KDoc note that only envelope-level max_memo_characters belongs in this property, so tighter message-level caps don't get folded back in.

3. Terra is Terra 2.0, limit 512 (id=3667442075)

Moved Chain.Terra to the 512 group with GaiaChain. It maps to CoinType.TERRAV2 (Chain.kt:123), i.e. phoenix-1, whose max_memo_characters is 512. TerraClassic (CoinType.TERRA, columbus-5) stays at the SDK default 256.

Tests

SendFormContinueGateTest updated: the ThorChain case now asserts 256 passes / 257 errors, the per-chain case covers Terra at 512, and a new a whitespace-only memo never blocks continue case covers #1.

Verification

• ./gradlew :app:ktfmtCheckMain :app:ktfmtCheckTest — passes. :data:ktfmtCheckMain reports Coins.kt and EvmCoinFinder.kt, both pre-existing on main and untouched by this PR.
• Unit tests could not be executed in this environment: :app:checkDebugAarMetadata fails resolving com.trustwallet:wallet-core:4.7.0 with 401 Unauthorized (no GITHUB_USER/GITHUB_TOKEN for the WalletCore package registry). CI will run them.

Declined: none.

[coderabbitai]

🧹 Nitpick comments (1)

app/src/test/java/com/vultisig/wallet/ui/models/send/SendFormContinueGateTest.kt (1)

90-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add a Terra Classic regression case.

This test covers Gaia/Terra 2.0 at 512 bytes and Noble at 256 bytes, but does not verify the explicitly corrected Terra Classic 256-byte limit. Add an over-limit assertion for Terra Classic so a future mapping regression is caught.

Proposed assertion

         assertNull(memoLengthErrorOrNull(Chain.GaiaChain, memo))
         assertNull(memoLengthErrorOrNull(Chain.Terra, memo))
         assertNotNull(memoLengthErrorOrNull(Chain.Noble, memo))
+        assertNotNull(memoLengthErrorOrNull(Chain.TerraClassic, memo))

As per the PR objective, Terra Classic remains at 256 bytes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@app/src/test/java/com/vultisig/wallet/ui/models/send/SendFormContinueGateTest.kt`
around lines 90 - 99, Extend the `limit is resolved per chain` test to assert
that a 300-character memo produces an error for `Chain.TerraClassic`, alongside
the existing Noble assertion, preserving the expected 512-character limits for
Gaia and Terra.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@app/src/test/java/com/vultisig/wallet/ui/models/send/SendFormContinueGateTest.kt`:
- Around line 90-99: Extend the `limit is resolved per chain` test to assert
that a 300-character memo produces an error for `Chain.TerraClassic`, alongside
the existing Noble assertion, preserving the expected 512-character limits for
Gaia and Terra.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 76f6823b-aea1-4d9a-8a63-0de61c0bbc4d

📥 Commits

Reviewing files that changed from the base of the PR and between ae927e7 and 9273e25.

📒 Files selected for processing (13)

• app/src/main/java/com/vultisig/wallet/ui/models/send/SendFormUiModel.kt
• app/src/main/res/values-de/strings.xml
• app/src/main/res/values-es/strings.xml
• app/src/main/res/values-hr/strings.xml
• app/src/main/res/values-it/strings.xml
• app/src/main/res/values-ko/strings.xml
• app/src/main/res/values-nl/strings.xml
• app/src/main/res/values-pt/strings.xml
• app/src/main/res/values-ru/strings.xml
• app/src/main/res/values-zh-rCN/strings.xml
• app/src/main/res/values/strings.xml
• app/src/test/java/com/vultisig/wallet/ui/models/send/SendFormContinueGateTest.kt
• data/src/main/kotlin/com/vultisig/wallet/data/models/Chain.kt

🚧 Files skipped from review as they are similar to previous changes (8)

• app/src/main/res/values-nl/strings.xml
• app/src/main/res/values-ko/strings.xml
• app/src/main/res/values-de/strings.xml
• app/src/main/res/values/strings.xml
• app/src/main/res/values-hr/strings.xml
• app/src/main/res/values-es/strings.xml
• app/src/main/res/values-zh-rCN/strings.xml
• data/src/main/kotlin/com/vultisig/wallet/data/models/Chain.kt

[Vaulty-bot]

Picked up CodeRabbit feedback (actionable issues only, skipping nitpicks). Working on it.

[Vaulty-bot]

Pre-merge check follow-up — 857564554

No new inline review comments came with this round (the actionable inline list was empty; the three earlier inline findings — blank-memo gating, THORChain's 250-byte cap, and Terra's 512-byte limit — were each answered and fixed in 9897199). What remained were the three pre-merge check warnings, handled as follows.

Fixed — Out of Scope Changes check

857564554 restores data/.../models/Coins.kt and data/.../usecases/EvmCoinFinder.kt to their main contents. Both had been reformatted by a repo-wide ./gradlew ktfmtFormat run rather than by any edit of mine — Coins.kt was pure re-wrapping of the STOCK_TOKENS declarations and EvmCoinFinder.kt a single lambda re-wrap. The PR diff is now exactly the memo validation, UI gating, localized strings and tests (16 files). Note those two files are unformatted with respect to ktfmt on main today, so a future ktfmtFormat run will reformat them again; that belongs in its own style commit, not here. Android CI (.github/workflows/android.yml) runs assembleDebug testDebugUnitTest, lintDebug and connectedDebugAndroidTest and does not gate on ktfmtCheck, so nothing regresses from leaving them as main has them.

Declined — Linked Issues check ("switch validation to character count")

Byte counting is the correct rule, and switching to characters would reopen the bug. The Cosmos SDK's ValidateMemo compares len(memo) — a Go string's UTF-8 byte length — against max_memo_characters; the parameter name is the misleading part. A 200-emoji memo is 800 for the node's purposes and is rejected at broadcast against a 256 ceiling, which is exactly the class of transaction #5435 is about. A character-count gate would pass it to keysign and burn the signing ceremony. This was raised as an inline comment earlier in the PR too, and CodeRabbit confirmed the byte-based behaviour as correct there and recorded it as a learning. The KDoc on Chain.maxMemoCharacters and on memoLengthErrorOrNull both state that the named parameter is enforced as a byte ceiling, and all ten locales describe the measured value as bytes rather than characters.

Declined — Docstring Coverage check (0.00%, threshold 80%)

Every public declaration this PR adds already carries KDoc: Chain.maxMemoCharacters, SendFormUiModel.isMemoBlocking, memoLengthErrorOrNull, plus an added paragraph in isContinueDisabled's existing KDoc. The 0.00% figure was measured over the diff while the Coins.kt reformatting was still in it — thousands of re-wrapped Coin(...) entries with no docs, dwarfing the documented additions. With that reformatting reverted the metric should reflect the actual new API. Per the repo's own guidance I have not added docstrings to code this PR does not change.

CI is re-running against the new head.

[johnnyluo]

LGTM

addressed