Only show memo input for Tendermint & ZHTLC#2998
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughConditional rendering added to show the memo field only for Tendermint and Zhtlc protocols. MemoField class removed from fill_form fields. Remaining changes are formatting and minor UI text restructuring without altering event payloads or behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant WF as WithdrawForm (UI)
participant B as WithdrawFormBloc
U->>WF: Open Withdraw Form
WF->>WF: Check asset.protocol
alt protocol is Tendermint or Zhtlc
WF-->>U: Show Memo Field
U->>WF: Edit Memo
WF->>B: Dispatch WithdrawFormMemoChanged
else
WF-->>U: Hide Memo Field
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Visit the preview URL for this PR (updated for commit 19b95ef): https://walletrc--pull-2998-merge-ans3luob.web.app (expires Tue, 19 Aug 2025 18:11:12 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@smk762 are ZHTLC and Tendermint the only coins with memo support? |
|
CharlVS
force-pushed
the
selective-memo-input
branch
from
21d2faa to
49948d0
Compare
smk762
force-pushed
the
selective-memo-input
branch
from
ccaec3c to
8f9de44
Compare
|
@coderabbitai review |
|
@cursor review |
|
@copilot review |
There was a problem hiding this comment.
Pull Request Overview
This PR implements conditional visibility for the memo input field in the withdraw form, restricting it to only appear for Tendermint and ZHTLC protocols while hiding it for EVM and UTXO networks.
- Added protocol-specific conditional rendering for the memo field
- Removed unused MemoField class from the codebase
- Applied code formatting improvements throughout the file
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| withdraw_form.dart | Added conditional rendering logic to show memo field only for Tendermint and ZHTLC protocols |
| fields.dart | Removed unused MemoField class and applied code formatting improvements |
| ], | ||
| ], | ||
| const SizedBox(height: 16), | ||
| if (state.asset.protocol is TendermintProtocol || state.asset.protocol is ZhtlcProtocol) ...[ |
There was a problem hiding this comment.
[nitpick] The protocol type checking logic should be extracted to a helper method or the asset model to improve maintainability and reduce duplication if this logic is needed elsewhere.
| if (state.asset.protocol is TendermintProtocol || state.asset.protocol is ZhtlcProtocol) ...[ | |
| if (_isMemoSupportedProtocol(state.asset)) ...[ |
There was a problem hiding this comment.
I refactored the memo field visibility logic within the withdraw form by extracting the protocol check into a dedicated helper function. This improves code readability and maintainability by centralizing the condition for memo support.
- I introduced a new private helper function,
_isMemoSupportedProtocol(Asset asset), inlib/views/wallet/coin_details/withdraw_form/withdraw_form.dart. This function encapsulates the logic to determine if an asset's protocol (TendermintProtocolorZhtlcProtocol) supports a memo field. - I updated the
WithdrawFormFillSectionwidget inlib/views/wallet/coin_details/withdraw_form/withdraw_form.dartto use the new_isMemoSupportedProtocol(state.asset)function. This replaced the inlineifcondition previously used to conditionally render theWithdrawMemoField, making the code cleaner and more explicit.
Learn more about Cursor Agents
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/fields.dart (1)
1-10: Remove unused widgets in fields.dartRG search shows that ConfirmationPage, SuccessPage, and FailurePage are only defined (no instantiations or references elsewhere). To reduce confusion and maintenance overhead, remove these dead widgets or extract them into separate files if needed later.
• lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/fields.dart
– ConfirmationPage (starts at line 293)
– SuccessPage (starts at line 409)
– FailurePage (starts at line 450)
🧹 Nitpick comments (4)
lib/views/wallet/coin_details/withdraw_form/withdraw_form.dart (2)
446-453: Memo field correctly gated to Tendermint and ZHTLC protocols (aligns with PR objective).This conditional render meets the requirement to show the memo only for Tendermint and ZHTLC assets. Nice.
Optionally, encapsulate the check to avoid future duplication and make it easier to extend (e.g., if other memo-capable protocols are added).
Apply this diff here:
-if (state.asset.protocol is TendermintProtocol || state.asset.protocol is ZhtlcProtocol) ...[ +if (_supportsMemo(state.asset.protocol)) ...[ WithdrawMemoField( memo: state.memo, onChanged: (value) => context .read<WithdrawFormBloc>() .add(WithdrawFormMemoChanged(value)), ), ],Add this helper inside WithdrawFormFillSection:
bool _supportsMemo(Protocol protocol) => protocol is TendermintProtocol || protocol is ZhtlcProtocol;
446-453: Verify coverage of all Tendermint-based protocol variantsI searched the local code for any additional
Tendermint*Protocolor*HTLC*Protocolclasses and found none. Since protocol implementations may live in dependencies or use different naming (e.g. token-specific or IBC variants), please manually confirm that all Tendermint-based asset protocols requiring a withdrawal memo are covered by this check. If you identify extra variants, update this condition accordingly:if (state.asset.protocol is TendermintProtocol || state.asset.protocol is ZhtlcProtocol) …[ WithdrawMemoField(...), ]lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/fields.dart (2)
196-224: EVM fee updates: consider input validation and keyboard/input formatters.Currently, negative or zero gas price/limit can be entered; also the gas price field uses a non-decimal keyboard. Small UX/validation improvements:
- Gas Price: allow decimals and restrict input to digits and a single dot.
- Gas Limit: restrict to digits only.
- Optionally validate min values (e.g., gas > 0, gasPrice > 0).
Apply these diffs:
UiTextFormField( labelText: 'Gas Price (Gwei)', - keyboardType: TextInputType.number, + keyboardType: const TextInputType.numberWithOptions(decimal: true), + inputFormatters: [ + FilteringTextInputFormatter.allow(RegExp(r'^\d*\.?\d{0,18}$')), + ], initialValue: evmFee?.gasPrice.toString(), onChanged: (value) { final gasPrice = Decimal.tryParse(value ?? ''); if (gasPrice != null) { context.read<WithdrawFormBloc>().add( WithdrawFormCustomFeeChanged( FeeInfoEthGas( coin: state.asset.id.id, gasPrice: gasPrice, gas: evmFee?.gas ?? 21000, ), ), ); } }, helperText: 'Higher gas price = faster confirmation', ), ... UiTextFormField( labelText: 'Gas Limit', - keyboardType: TextInputType.number, + keyboardType: TextInputType.number, + inputFormatters: [FilteringTextInputFormatter.digitsOnly], initialValue: evmFee?.gas.toString() ?? '21000', onChanged: (value) { final gas = int.tryParse(value ?? ''); - if (gas != null) { + if (gas != null && gas > 0) { context.read<WithdrawFormBloc>().add( WithdrawFormCustomFeeChanged( FeeInfoEthGas( coin: state.asset.id.id, gasPrice: evmFee?.gasPrice ?? Decimal.one, gas: gas, ), ), ); } }, helperText: 'Estimated: 21000', ),
315-361: Also gate memo display on confirmation to supported protocols.Even if this page is not the active confirm screen, it’s safer to avoid showing memo details for assets that don’t support memos. Gate by protocol, and optionally require non-empty memo.
Apply this diff:
- if (state.memo != null) ...[ + if ((state.memo?.isNotEmpty ?? false) && + (state.asset.protocol is TendermintProtocol || + state.asset.protocol is ZhtlcProtocol)) ...[ const SizedBox(height: 12), _ConfirmationItem(label: 'Memo', value: state.memo!), ],If we adopt the helper suggested in withdraw_form.dart, we can reuse it here too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/fields.dart(18 hunks)lib/views/wallet/coin_details/withdraw_form/withdraw_form.dart(1 hunks)
🔇 Additional comments (10)
lib/views/wallet/coin_details/withdraw_form/withdraw_form.dart (1)
19-20: No staleMemoFieldreferences detectedI ran a project-wide search for
MemoFieldand only found the newWithdrawMemoFieldusages. There are no lingering imports or references to the oldMemoField, so this is safe to resolve.lib/views/wallet/coin_details/withdraw_form/widgets/fill_form/fields/fields.dart (9)
30-33: Event dispatch formatting change is fine.No behavior change; dispatch remains the same.
66-97: Decimal keyboard for Amount + event dispatch refactor look good.
- Enabling decimal input on Amount is correct for most assets.
- Dispatch logic is unchanged, now cleaner.
118-121: Network fee title refactor is fine.Semantics preserved with better readability.
166-167: Custom fee toggle dispatch formatting is fine.No behavioral change.
266-276: UTXO fee selection mapping looks correct.
- Selected set reflects current or default fee.
- Event payload uses Decimal.fromInt, which is appropriate for satoshi-like integers.
386-403: Style update reads well.Using colorScheme.onSurface with a reduced alpha for the label is consistent with other components. Approved.
468-485: Failure styling + event dispatch formatting are fine.No semantic change; readability improved.
507-509: IBC toggle event dispatch formatting is fine.No behavior change.
532-533: IBC channel onChanged formatting is fine.No behavior change.
Co-authored-by: charl <charl@vanstaden.info>
|
@cursoragent resolve the cause of the build failures |
|
I refactored the pubkey watching mechanism within the
Learn more about Cursor Agents |
CharlVS
force-pushed
the
selective-memo-input
branch
from
4652c04 to
e3cfe79
Compare
* limit memo field to supporting protocols * add TODO for unused component * removes obsolete `MemoField` * Add helper function to check memo support for specific protocols Co-authored-by: charl <charl@vanstaden.info> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: charl <charl@vanstaden.info> Co-authored-by: CharlVS <77973576+CharlVS@users.noreply.github.com>
Closes #2997
To Test:
Summary by CodeRabbit
Bug Fixes
Style