request Destination tag for ripple #16915
Conversation
|
🚀 Expo preview is ready!
|
|
Why is dbdd244 part of this PR? It is probably mistake |
| /> | ||
| </Animated.View> | ||
| )} | ||
| {isInputDisplayed && ( |
There was a problem hiding this comment.
nit: isInputDisplayed ternanry operator is already used few lines above. I would prefer if this code block would be rendered under the same condition next to the TextInputField to avoid condition duplication.
There was a problem hiding this comment.
As a benefit it can be a single Animated.View. We also have AnimatedBox and AnimatedVStack defined, that might come handy to you.
| @@ -194,10 +194,36 @@ export const sendOutputsFormValidationSchema = yup.object({ | |||
| }), | |||
| ) | |||
| .required(), | |||
| enableDestinationTagValidation: yup.boolean(), | |||
There was a problem hiding this comment.
nit: what about this name?
| enableDestinationTagValidation: yup.boolean(), | |
| isRippleDestinationTagEnabled: yup.boolean(), |
| .matches(/^\d*$/, 'You can only use positive numbers for the destination tag.') | ||
| .test( | ||
| 'is-destination-tag-set', |
There was a problem hiding this comment.
nit: I would change name of this validation to this:
| 'is-destination-tag-set', | |
| 'is-destination-tag-required', |
Because it runs even for non XRP coins where it is not required.
vytick
changed the title
vytick
force-pushed
the
icons-tokens-with-networks
branch
from
6bc67e3 to
39c7218
Compare
trezor-ci
force-pushed
the
icons-tokens-with-networks
branch
from
39c7218 to
6f6ceab
Compare
vytick
force-pushed
the
feat/native/xrp-destination-tag-enhancement
branch
from
d44a491 to
b23fb40
Compare
WalkthroughThe changes across the modules introduce enhanced support for XRP transactions by updating both UI components and validation logic. The localization strings now include detailed instructions and warnings for XRP destination tags, while the label for destination tags in the sending module is restructured into an object with multiple descriptive fields. Changes in the ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (3)
suite-native/module-send/src/components/DestinationTagInput.tsx (1)
45-57: Consider adding error handling for focus timing.The setTimeout for input focus has no error handling and might fail silently if the input ref is not ready.
const handleShowInputChange = () => { if (!isInputDisplayed) { setValue(isRippleDestinationTagEnabledName, true); - // Wait for input element to be mounted. - setTimeout(() => { + const focusTimeout = setTimeout(() => { inputRef.current?.focus(); - }); + }, 0); + return () => clearTimeout(focusTimeout); } else { setValue(isRippleDestinationTagEnabledName, false); } trigger(destinationTagFieldName); setIsInputDisplayed(!isInputDisplayed); };suite-native/module-send/src/screens/SendOutputsScreen.tsx (1)
211-216: Address the TODO comment about using reset().The code contains a TODO suggesting to use reset() instead of setValue().
Would you like me to help implement the form reset functionality to replace the setValue() calls?
suite-native/intl/src/en.ts (1)
1135-1141: LGTM! Well-structured translations for destination tag fields.The translations are clear, informative, and properly structured with label, warning, info, and linkText fields. The messages effectively guide users through the destination tag requirement for exchange transactions.
Consider adding a placeholder text for the destination tag input field to provide an example of what a valid tag looks like.
Add a placeholder field to the
destinationTagobject:destinationTag: { label: 'Destination tag', warning: 'Online exchanges require this to identify your account. Get your destination tag from your Ripple account. Make sure you really don't need it.', info: 'Online exchanges require this to identify your account. Get your destination tag from your exchange.', linkText: '<link>What's this?</link>', + placeholder: 'e.g., 123456', },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
suite-native/intl/src/en.ts(2 hunks)suite-native/module-send/src/components/DestinationTagInput.tsx(2 hunks)suite-native/module-send/src/screens/SendOutputsScreen.tsx(2 hunks)suite-native/module-send/src/sendOutputsFormSchema.ts(1 hunks)suite-native/receive/src/screens/ReceiveAddressScreen.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
suite-native/module-send/src/sendOutputsFormSchema.ts
[error] 202-202: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (6)
suite-native/receive/src/screens/ReceiveAddressScreen.tsx (1)
70-115: LGTM! Well-structured implementation of XRP info alert.The implementation effectively adds an informative alert for XRP accounts with proper localization and helpful documentation link.
suite-native/module-send/src/components/DestinationTagInput.tsx (1)
111-138: Consolidate animation logic to reduce duplication.The animation logic is duplicated between the input and alert box sections.
As suggested in the previous review, consider consolidating the animation logic under a single condition to avoid duplication.
suite-native/module-send/src/sendOutputsFormSchema.ts (2)
197-204: LGTM! Well-structured conditional validation.The validation logic correctly handles the conditional requirement of the destination tag based on the enabled state.
🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Do not add then to an object.
(lint/suspicious/noThenProperty)
206-226: Consider renaming the test for clarity.The test name 'is-destination-tag-required' could be misleading as it runs for non-XRP coins where it's not required.
As suggested in the previous review, consider renaming to better reflect its purpose.
suite-native/module-send/src/screens/SendOutputsScreen.tsx (1)
119-122: LGTM! Clean implementation of default values.The implementation correctly sets the default state of the destination tag based on the network type.
suite-native/intl/src/en.ts (1)
520-521: LGTM! Clear and helpful message for XRP destination tags.The message effectively communicates that while exchanges may require destination tags, Trezor itself does not, and provides a link for more information.
| const debounce = useDebounce(); | ||
|
|
||
| const { trigger } = useFormContext<SendOutputsFormValues>(); | ||
| const isRippleDestinationTagEnabledName: SendFieldName = 'isRippleDestinationTagEnabled'; |
There was a problem hiding this comment.
| const isRippleDestinationTagEnabledName: SendFieldName = 'isRippleDestinationTagEnabled'; | |
| const isRippleDestinationTagEnabledFieldName: SendFieldName = 'isRippleDestinationTagEnabled'; |
There was a problem hiding this comment.
good spot, fixed and rebased
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13261538955 |
trezor-ci
force-pushed
the
feat/native/xrp-destination-tag-enhancement
branch
from
061645b to
8b0b18e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
suite-native/module-send/src/components/DestinationTagInput.tsx (1)
111-138: Consider consolidating the animation components.The animation logic is duplicated between the input field and alert box sections. Consider using a single
AnimatedVStackorAnimatedBoxto wrap both conditions.- {isInputDisplayed ? ( - <AnimatedVStack spacing="sp8" entering={FadeIn} exiting={FadeOut}> - <TextInputField - valueTransformer={integerTransformer} - ref={inputRef} - onChangeText={handleChangeValue} - name={destinationTagFieldName} - testID={destinationTagFieldName} - onFocus={handleInputFocus} - accessibilityLabel="address input" - /> - <HStack paddingHorizontal="sp12" spacing="sp4"> - <Icon name="info" color="iconSubdued" size="medium" /> - <Text variant="label" color="textSubdued"> - <Translation id="moduleSend.outputs.recipients.destinationTag.info" /> - </Text> - </HStack> - </AnimatedVStack> - ) : ( - <Animated.View entering={FadeIn} exiting={FadeOut}> - <AlertBox - variant="warning" - title={ - <Translation id="moduleSend.outputs.recipients.destinationTag.warning" /> - } - /> - </Animated.View> - )} + <AnimatedVStack entering={FadeIn} exiting={FadeOut}> + {isInputDisplayed ? ( + <> + <TextInputField + valueTransformer={integerTransformer} + ref={inputRef} + onChangeText={handleChangeValue} + name={destinationTagFieldName} + testID={destinationTagFieldName} + onFocus={handleInputFocus} + accessibilityLabel="address input" + /> + <HStack paddingHorizontal="sp12" spacing="sp4"> + <Icon name="info" color="iconSubdued" size="medium" /> + <Text variant="label" color="textSubdued"> + <Translation id="moduleSend.outputs.recipients.destinationTag.info" /> + </Text> + </HStack> + </> + ) : ( + <AlertBox + variant="warning" + title={ + <Translation id="moduleSend.outputs.recipients.destinationTag.warning" /> + } + /> + )} + </AnimatedVStack>suite-native/module-send/src/sendOutputsFormSchema.ts (2)
197-204: Fix the Yup schema syntax to avoid thethenproperty warning.The current implementation triggers a static analysis warning about using the
thenproperty. Consider using the alternative syntax for conditional validation.- rippleDestinationTag: yup - .string() - .when('isRippleDestinationTagEnabled', { - is: true, - then: schema => schema.required('Destination Tag is required'), - otherwise: schema => schema.notRequired(), - }) + rippleDestinationTag: yup + .string() + .when('isRippleDestinationTagEnabled', ([isEnabled], schema) => + isEnabled + ? schema.required('Destination Tag is required') + : schema.notRequired() + )🧰 Tools
🪛 Biome (1.9.4)
[error] 202-202: Do not add then to an object.
(lint/suspicious/noThenProperty)
206-226: The validation test name could be more descriptive.The test name 'is-destination-tag-required' doesn't fully convey its purpose since it runs for all coins but only enforces the tag for Ripple.
- 'is-destination-tag-required', + 'is-ripple-destination-tag-required',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
suite-native/intl/src/en.ts(2 hunks)suite-native/module-send/src/components/DestinationTagInput.tsx(2 hunks)suite-native/module-send/src/screens/SendOutputsScreen.tsx(2 hunks)suite-native/module-send/src/sendOutputsFormSchema.ts(1 hunks)suite-native/receive/src/screens/ReceiveAddressScreen.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- suite-native/module-send/src/screens/SendOutputsScreen.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
suite-native/module-send/src/sendOutputsFormSchema.ts
[error] 202-202: Do not add then to an object.
(lint/suspicious/noThenProperty)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (3)
suite-native/receive/src/screens/ReceiveAddressScreen.tsx (1)
70-115: LGTM! The XRP destination tag information is well-integrated.The changes effectively integrate the XRP destination tag information into the receive screen:
- The condition for showing XRP info is correctly implemented
- The alert box provides clear information with a properly configured link
- The UI components are well-structured and follow the design system
suite-native/module-send/src/components/DestinationTagInput.tsx (1)
37-57: The state management and form context updates are well-implemented.The toggle functionality is properly implemented with appropriate form context updates and state management.
suite-native/intl/src/en.ts (1)
520-522: LGTM! The translation strings are clear and well-structured.The new translation strings effectively communicate the purpose and requirements of the XRP destination tag feature:
- Clear instructions for users
- Proper explanation of when tags are required
- Well-structured messages with appropriate placeholders
Also applies to: 1135-1141
By default request Destination tag for ripple
Related Issue
Resolve #16636
Screenshots:
Simulator.Screen.Recording.-.iPhone.16.-.2025-02-11.at.01.29.41.mp4