Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #4677: Prefill default buy/send/swap token if user comes from token details screen #4756

Merged
merged 5 commits into from
Jan 12, 2022

Conversation

nuo-xu
Copy link
Contributor

@nuo-xu nuo-xu commented Dec 20, 2021

This pull request fixes #4677

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Case 1:

  1. Go to buy/send/swap screen from Asset Detail Screen
  2. Make sure the prefilled buy token, or send from token, or swap from token is the same as the token from the asset details screen (swap to token should not be the same as the swap from token)

Case 2:

  1. Make sure the selected network is NOT Ropsten
  2. Go to buy/send/swap screen from anywhere but NOT Asset Detail Screen
  3. Make sure prefilled buy token is BAT, prefilled send from token is ETH, prefilled swap from token is ETH, the prefilled swap to token is BAT

Case 3:

  1. Make sure the selected network is Ropsten
  2. Go to send/swap screen from anywhere but NOT Asset Detail Screen
  3. prefilled send from token is ETH, prefilled swap from token is ETH, prefilled swap to token is DAI

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue and pull request is assigned to a milestone (should happen at merge time).

@soner-yuksel
Copy link
Contributor

BraveWalletTests are failing.

Comment on lines 12 to 18
enum BuySendSwapDestination: Identifiable, CaseIterable, Equatable, Hashable {

static var allCases: [BuySendSwapDestination] = [.buy(), .send(), .swap()]

case buy(BraveWallet.ERCToken? = nil)
case send(BraveWallet.ERCToken? = nil)
case swap(BraveWallet.ERCToken? = nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all of these take the same arg, it doesn't make sense to use an enum w/ associated value. Better to keep BuySendSwapDestination the same and put it inside a container struct i.e.

struct BuySendSwapDestination: Identifiable, Equatable, Hashable {
   enum Kind: Identifiable, CaseIterable, Equatable, Hashable { case buy, send, swap }
   var kind: Kind
   var initialToken: BraveWallet.ERCToken?
   var id: String { kind.id }
}

@nuo-xu
Copy link
Contributor Author

nuo-xu commented Dec 21, 2021

BraveWalletTests are failing.

fixed the unit tests :)

Copy link
Collaborator

@kylehickinson kylehickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Maybe add some unit tests for prefilled tokens to ensure the selected tokens are correct.
  2. If a user taps "buy" from a token that doesn't exist in the buy list supported by Wyre don't we need some sort of validation there before setting it as the selectedBuyToken?

@nuo-xu
Copy link
Contributor Author

nuo-xu commented Dec 21, 2021

hey @kylehickinson

  1. I will add unit tests to test prefilled tokens
  2. This is clearly mobile only UX problem since desktop has no such flow that you click into token details and then go to buy token from there. So on desktop, the panel will only give you a list of Wyre supported tokens from TokenRegistery.buyTokens. I've tried this case on the current mobile flow(I added a random ERC20 token to my visible assets and try to buy this token from the token details screen), since the buy url is generated using account address and the token symbol, the url is still valid with website UI also makes sense. It is just going to be impossible for you to buy this token. I think this is acceptable behaviour for now.
    Screenshot:
    IMG_87611756725F-1

@nuo-xu nuo-xu modified the milestones: 1.34, 1.35 Dec 23, 2021
@soner-yuksel soner-yuksel modified the milestones: 1.34, 1.35 Jan 4, 2022
@iccub
Copy link
Contributor

iccub commented Jan 10, 2022

Needs test plan

@nuo-xu
Copy link
Contributor Author

nuo-xu commented Jan 12, 2022

Needs test plan

Test plan added

@nuo-xu nuo-xu merged commit 8ae3f1c into development Jan 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selecting Swap from a token doesn't set it as the swap from token
5 participants