-
Notifications
You must be signed in to change notification settings - Fork 38
Add memo-required flows for Dapp + Normal send #2400
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/5.37.0
Are you sure you want to change the base?
Conversation
* upgrade to yarn 2 and use resolutions to block vulnerable package versions (#2239) * upgrade to yarn 2 and use resolutions to block vulnerable package versions * rm deprecated .yarnrc * rm yarnpath * try committing yarn binary to repo * try corepack enable for gha * update run tests cmd * rm yarnpath * rm npm i yarn * update all pipelines * rm superfluous history types * ensure invoke host function tx shows contract parameters (#2243) * ensure invoke host function tx shows contract parameters * add test for fallback if contract spec retrieval fails * Bugfix/rm auth param names (#2244) * ensure invoke host function tx shows contract parameters * add test for fallback if contract spec retrieval fails * do not show contract parameters for authorizations * add tests for create contract v1 and invoke contract * add issuer for changeTrust op (#2246) * add issuer for changeTrust op * programmatically disable overflow:hidden when copying a value * Revert "add issuer for changeTrust op (#2246)" (#2247) This reverts commit 19c8a68. * Bugfix/add issuer for changetrust (#2249) * ensure invoke host function tx shows contract parameters * add test for fallback if contract spec retrieval fails * do not show contract parameters for authorizations * add tests for create contract v1 and invoke contract * add issuer for changeTrust op * programmatically disable overflow:hidden when copying a value * cache account balances and poll for updates * fix CI tests * rm `force:true` which was causing action to happen too fast * do a fresh balance fetch on account/network change * first pass at async history * pr comments * allow for history caching * add more sentry tracking for Account and Wallets views (#2268) * add more sentry tracking for Account and Wallets views * adding more sentry reporting * gracefully degrade on errors from Blockaid (#2269) * gracefully degrade on errors from Blockaid * should not be necessary to skip dapp scanning on custom network * rm extra dep * add a test for persisting configurations in the send flow (#2271) * add a test for persisting configurations in the send flow * rm logs and update muxed acct; lower xlm payment * rm slow loading simulation * handle missing scan-tx result; add disabled state for Confirm Anyway (#2272) * handle missing scan-tx result; add disabled state for Confirm Anyway * assertions to show correct confirm button on Blockaid error * add cache for balances to ensure we do a fresh lookup when needed (#2275) * add cache for balances to ensure we do a fresh lookup when needed * add try...catch to token-prices polling * rm log * only dispatch saveBalancesForAccount when fresh data has been fetched * adjust test to wait for UI change * replace yarn setup with just yarn * rm unnecessary return * clear token details on redux clear action * make history row construction async and check for redux state for updates * add tests for assetdetails * increase timeout for flakey test * pr comments * refresh account history every time account balances refresh
* check for updated appdata before showing password modal * update error msg * rm unused redirect logic
* move get icons out of critical path; rely on background's cache * add tests and comments * add comment * add comment * only dispatch if we have cached icons * PR comments
* skip blockaid scan on first fetch of account-balances * rm stub change * rm more stubs * rm log * add comments and update boolean naming
* add button to copy address from dropdown * Added translations * revert translation file changes * revert translation file changes * Added translations * revert changes to translation files * move copy address button to first dropdown position
* scroll on long strings; pretty print json * rm log * add correct snapshot for json message * rm log * finish comment * add error case for JSON * don't use carat for lib * update yarn.lock * move scrollbar to btm of container; reduce json font size * update snapshot
* re-searching so should abort any in flight API requests * add comment * fix test name * make test more reliable * add check for correct search results * fix jest locator
* adds SelectionTile and AddressTile, updates nav flows to match updates. Adds query parameter for default values in send flow * Added translations * adds address tile and uses it in swap flow, tweaks selection tile styles * adds unit tests for new tile components * Added translations * updates swap navigation flow to match updates, updates tests flows to match * updates back icon for send and swap steps, fixes bad test references * tweaks locator in address tile tests * adds store state to asset tile tests, removes asset icon mock * updates SelectionTile prop name, adds isSuspicious prop for AssetTile * adds placeholder value in TokenList for missing token USD value * uses real IdenticonImg in address tile unit tests * adds query param validation for send and swap flow * Update extension/src/popup/views/SendPayment/index.tsx Co-authored-by: Cássio Marcos Goulart <[email protected]> * adds missing import * adds class for tile icon --------- Co-authored-by: Cássio Marcos Goulart <[email protected]>
* adds send and swap buttons to asset detail view * uses secondary button styles * removes run snapshots job (#2355) * release/5.35.4 (#2354) * upgrade to ledger-hq/hw-transport-webhid (#2350) * upgrade to ledger-hq/hw-transport-webhid * add tests * add ledger support for new trustline flow (#2352) * upgrade to ledger-hq/hw-transport-webhid * add ledger support for new trustline flow * only re-fetch balances if we were successful * test for fetching balances on success * add reset spys * adjust spacing at top of hw wallet modal * Now that `Done` button properly shows, click it in tests (#2356) * skip flakey test * skip flakey test * renames local vars to follow convention * adds tests for LP share and tweaks LP title * adds links with query params for asset detail CTAs --------- Co-authored-by: Piyal Basu <[email protected]>
* only fetch asset list data if needed * correctly show icon loading state
* adds SAC detection when changing trust in the add and remove token flows * updates arg signature for isAssetSac
# Conflicts: # extension/src/popup/locales/en/translation.json # extension/src/popup/locales/pt/translation.json
* cache token prices and batch loading wallets * use similar methodology for token price and account balance caching * fix loading state trigger * fix tests * use helper for cache clearing * set isFetchingTokenPrices to false in catch handler * rollback error change
* load backend settings async on Account view * rm console.logs
* use ledger-key/accounts endpoint for home domains * fix tests * create generic ledger key account helper; add tests * rm unneeded data-test prop * rm unused import * [CHORE] git process updates (#2361) * moves the add translations hook to the pre commit stage, removes standalone translations commit * adds script to update app version, removes version update from submit production action, adds update version step to test run action for release branches * removes version input, now uses package version * fetch asset domains in one calls * fix tests * cache home domains while iterating over account history rows * fitler non-G keys * rm .only * fix test param * PR comments --------- Co-authored-by: aristides <[email protected]>
…ature/check-memo-required-on-send
…ature/check-memo-required-on-send
|
@leofelix077 it looks like this state should be within a view container in order to respect the max width in full screen mode. Typically, for the labels with the right facing caret we have made the interaction slide right to a pane within the bottom sheet itself. The transaction details label or the blockaid labels when signing a transaction should be good references for this.
|
* use redux selector for allAccounts to properly update rename * add longer timeout for flakey btn
…ature/check-memo-required-on-send
Screen.Recording.2025-11-21.at.14.37.06.mov |
…ature/check-memo-required-on-send
| for (let i = 0; i < shuffledWords.length; i++) { | ||
| await page.getByLabel(shuffledWords[i]).check({ force: true }); | ||
| // Use nth() to handle duplicate labels by selecting the first matching element | ||
| await page.getByLabel(shuffledWords[i]).first().check({ force: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, does this work in the case of duped words? I would think from this code it would never select the 2nd instance of a word so if the phrase was "foo bar bar", it would end up just selecting the first instance of "bar" and it would try to validate "foo bar", which would fail
| * Effect to parse XDR and set initial memo validation state | ||
| * Runs when XDR, network, or validation settings change | ||
| */ | ||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should be able to consolidate these next 2 useEffect calls - they seem duplicative. You should be able to just call checkIsMemoRequired after you build the xdr and set hasMemo`
| setIsMemoMissing(true); | ||
| setIsValidatingMemo(false); | ||
|
|
||
| if (!shouldValidateMemo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIt: I think we can also combine these next 2 if statements - if (!shouldValidateMemo || (!xdr || !networkDetails)) since they do the same thing
| : t("None")} | ||
| </span> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense - but the Figma design actually doesn't include the memo here. It shows it on the Transaction Details view. Let's check with @sdfcharles to see what he thinks about adding the memo here
| // Regenerate transaction XDR with new memo (now reads memo from Redux state inside fetchData) | ||
| await fetchSimulationData(); | ||
| // Reopen review sheet after memo is saved and XDR is regenerated | ||
| setIsReviewingTx(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, why automatically open this review sheet here? As a user, I would expect this to come up only after I click Review Send. It's also a bit jarring because editing the fee doesn't open the review sheet - it's just editing the memo.
I think we should maintain the original UX of the user needing to click Review Send to get to the review sheet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm noticing a bug where if I'm sending to a memo required address, if I enter a memo, I don't see the memo warning (great!), but if I go back and clear the memo, I can get past the memo warning
|
Let's also add some tests for this functionality |

normal-memo-send.mov
dapp-memo-send.mov
memo-max-bytes-local-validation.mov