Skip to content
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

Update send flow "recents address" list #2912

Closed
omnat opened this issue Jul 17, 2021 · 1 comment · Fixed by #3028
Closed

Update send flow "recents address" list #2912

omnat opened this issue Jul 17, 2021 · 1 comment · Fixed by #3028
Assignees
Labels
this issue is prioritized Story in develop or in release type-bug Something isn't working

Comments

@omnat
Copy link
Contributor

omnat commented Jul 17, 2021

Description

Currently in the Send flow, under the list of “recent addresses”, you see all the addresses that users have interacted with and that list is populated from the transaction history (TransactionController). This doesn’t match with user expectation, and could lead to users sending assets to addresses they didn’t intend to. We should not be using the transactions from the TransactionController.

Technical Details

  • Store all sent TO addresses in a list under a new key in redux. Perhaps call it recent-addresses. Ordered from most recent -> oldest
  • Replace transactions from AddressList component with address list from recent-addresses.
  • Depending on the model of a transaction coming from TransactionController, you may want to model the items under recent-addresses in a similar manner to reduce potential UI changes. The other option, for the sake of cleaning things up, would be to simplify the model.
  • Most likely, this feature will require a new action creator + selector in redux.
  • Lastly, we'll want to limit the number of addresses to display. A reasonable number would be 20 or so. That means if the list gets too long, we can pop off the end and add new addresses to the front.
  • May need to update unit tests

Acceptance Criteria

  • Recent addresses should ONLY show addresses that users have sent to via the “Send” feature in the past (in the order: most recent “to address” top of the list).
  • Ability to select from recent address list and populate the TO field.
  • Confirm sending still works after selecting from address list.

References

Questions

  • Does it make sense to have the ability to remove addresses from the list? For example: Have an X on the right.
    @mobularay : out of scope for this issue and sprint.
Currently in the Send flow, under the list of “recent addresses”, you see all the addresses that users have interacted with and that list is populated from the transaction history. This doesn’t match with user expectation, and could lead to users sending assets to addresses they didn’t intend to. 

### **Fix:** 
In the ‘recent addresses’ list of Send flow, ONLY show the addresses that were previously added & sent to in the “recipient address field”.

### **QA paths to test:**

1. this should solve the issue where people are seeing Swaps contract address (and other contract addresses after interaction with sites) in the “recent addresses”
2. this should allow contract addresses if the user had intentionally sent to a contract addresses via the Send flow (e.g., to another contract account wallet, or even for wrapping ETH)
3. “Recent addresses” should ONLY show addresses that users have sent to via the “Send” feature in the past (in the order: most recent “to address” top of the list). 

### **Acceptance criteria**

1. Store this information in a privacy respecting way
2. Minimize performance impact
3. No confusion-inducing UX impact

Related: Ticket (#2822) & PR https://github.com/MetaMask/metamask-mobile/pull/2823 (stop-gap solution) ```
@omnat omnat added the type-bug Something isn't working label Jul 17, 2021
@mobularay
Copy link
Contributor

mobularay commented Jul 19, 2021

The engineer that will be picking up this work, can reach out to Ricky for more info

Other requirements to nail down:

  • where/how to store this growing info? what's the ceiling? what is the current number of addresses displayed on the UI?
  • what level of encryption is needed?
  • how is the app going to be backwards compatibility for users when move to this version, will not see the list of addresses anymore, until new txns occur?

Need more grooming. An Engineer to lead.

@Cal-L Cal-L changed the title Fix "Recent addresses" functionality in the Send flow Update SendFlow address list Jul 21, 2021
@Cal-L Cal-L changed the title Update SendFlow address list Update send flow address list Jul 21, 2021
@omnat omnat added the this issue is prioritized Story in develop or in release label Jul 28, 2021
@mobularay mobularay changed the title Update send flow address list Update send flow "recents address" list Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
this issue is prioritized Story in develop or in release type-bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants