feat: MUSD-150 quick convert (Part 3)#26638
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
82b4e28 to
2e1b2f8
Compare
| style={styles.container} | ||
| testID={MusdMaxConversionInfoTestIds.CONTAINER} | ||
| > | ||
| <MusdMaxConversionAssetHeader |
There was a problem hiding this comment.
Can we not rely on the standard Title component in all the confirmations?
Is this conceptually a row in which case could we rename and move to the rows directory?
There was a problem hiding this comment.
Here's what this component looks like. It's not really a row but meant to be used as a header. I'll see if there's a better place for this to live.
There was a problem hiding this comment.
@matthewwalsh0 I've refactored this component to make it more reusable and have moved it out of the musd-conversion namespace.
There was a problem hiding this comment.
I feel "Row" has become a very lose definition to meaning a rendering of data inside a confirmation, so no specific rules on what it looks like.
Apologies for the confusion, I don't have a problem if it's MUSD specific, but great if we've made it more generic.
My only preference now would be to remove the props from info so it locates the tokens itself.
So you for example make a MUSD specific row component that wraps the new generic one?
… relying on props
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
This reverts commit 70f0bf7.
There was a problem hiding this comment.
Nothing really blocking, just some architecture preferences to favour modular components that retrieve their own data.
We can hopefully address in future, don't want to excessively delay this.
Appreciate the work done on creating the rate row and alert components! ❤️
| </View> | ||
| <BlockingAlertMessage /> | ||
| <View style={styles.buttonContainer}> | ||
| <Button |
There was a problem hiding this comment.
Agree we can abstract this later if needed, but maybe just a local ConfirmButton in the same file so you can encapsulate more of the logic to minimise any code in the info component itself?
| testID={MusdMaxConversionInfoTestIds.CONTAINER} | ||
| > | ||
| <TokenConversionAssetHeader | ||
| inputToken={token} |
There was a problem hiding this comment.
Same here, if this asset header is unique to this use case, could we encapsulate data retrieval in there?
| style={styles.container} | ||
| testID={MusdMaxConversionInfoTestIds.CONTAINER} | ||
| > | ||
| <MusdMaxConversionAssetHeader |
There was a problem hiding this comment.
I feel "Row" has become a very lose definition to meaning a rendering of data inside a confirmation, so no specific rules on what it looks like.
Apologies for the confusion, I don't have a problem if it's MUSD specific, but great if we've made it more generic.
My only preference now would be to remove the props from info so it locates the tokens itself.
So you for example make a MUSD specific row component that wraps the new generic one?
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection: New Components Added:
Modified Components:
Why SmokeConfirmations:
Why not other tags:
Performance Test Selection: |
The committed fixture schema is out of date. To update, comment: |
Description
The third and final Quick Convert foundational PR. This adds the mUSD Max conversion bottom sheet and mUSD conversion info root which routes to either quick convert or custom convert based on the
forceBottomSheetroute param.Changelog
CHANGELOG entry: mUSD quick convert (part 3) adding max convert bottom sheet
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Medium Risk
Touches transaction confirmation UI/flow for
TransactionType.musdConversion, including confirm enablement/labeling based on quotes and blocking alerts. Risk is moderate because regressions could prevent users from confirming conversions or hide required warnings.Overview
Adds a new mUSD max-conversion confirmation UI. Introduces
MusdMaxConversionInfo(asset header + rate/fee/total/percentage rows) and a newMusdConversionInfoRootthat routes between the existing conversion view and the new max-convert bottom sheet based on theforceBottomSheetnav param.Improves blocking-alert UX in confirmations. Adds reusable
BlockingAlertMessageto render the first blocking alert’s message and updates the max-convert confirm button to disable when loading or blocked and to use the blocking alert title as the button label.Polishes related confirmation wiring/tests. Updates the confirmation title for
musdConversionto “Convert max”, adds aTokenConversionRateRow, adjustsFooterQR hardware context import and removes unusedconfirmDisabledstyling param, and adds/updates unit tests and locale strings accordingly.Written by Cursor Bugbot for commit 3102d98. This will update automatically on new commits. Configure here.