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 on-ramp providers and add purchase method screen #16872

Merged
merged 36 commits into from
Feb 11, 2023
Merged

Conversation

simoarpe
Copy link
Collaborator

@simoarpe simoarpe commented Jan 26, 2023

Resolves brave/brave-browser#23316 (Update on-ramp methods)

Resolves brave/brave-browser#27996 (Switch to new API getBuyUrlV1)

Implements a new screen to select a purchase method containing Ramp.Network, Sardine and Transak.
Adopted new API getBuyUrlV1.

  • ⚠️ Sardine will appear only in US locales.
  • Ramp.Network needs modified symbols obtained by mapToRampNetworkSymbol.
  • New core method that accepts an array of providers, and returns the full list.
  • Implemented removeDuplicates to avoid showing duplication in the EditVisibleAssetsBottomSheetDialogFragment UI.
  • Update Sardine with new logo.
Screenshot 2023-01-31 at 1 29 34 PM Screenshot 2023-01-31 at 1 31 30 PM

Demo

demo.mov

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Open the wallet section
  • Tap on FAB icon and select "Buy"
  • Tap on "Select purchase method" button
  • Observe the new screen "Select purchase method"
  • Verify that tapping on the "Buy with Ramp" button opens a new page on Ramp network
  • Verify that tapping on the "Buy with Sardine" button opens a new page on Sardine
  • Verify that tapping on the "Buy with Transak" button opens a new page on Transak

@simoarpe simoarpe added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Jan 26, 2023
@simoarpe simoarpe self-assigned this Jan 26, 2023
@Pavneet-Sing
Copy link
Contributor

I believe the primary task with on-ramp implementation is to expand the support by adding additional Ramp networks(Sardine etc). The UI is good to have but it was previously required due to multiple options: ramp and wyre.
As of now, it's just an extra step to select the only one option available.
Isn't it better to keep the previous implementation to use ramp as a default option for buy?
Is there any plan to add more network options to this screen?
cc: @jamesmudgett @SergeyZhukovsky

@simoarpe
Copy link
Collaborator Author

Is there any description of the actual changes required? I copied the iOS flow but couldn't find any other differences other than the extra screen "Select purchase method".

@@ -175,6 +175,7 @@ brave_java_resources = [
"java/res/drawable-nodpi/ic_quick_action_search_and_bookmark_widget_preview.png",
"java/res/drawable-nodpi/loader0.png",
"java/res/drawable-nodpi/loader0_orange.png",
"java/res/drawable-nodpi/ramp.png",
Copy link
Member

Choose a reason for hiding this comment

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

could we have it in svg format? We are going to convert all pictures to vector graphic soon. That will decrease resources size plus they are scalable. It doesn't make sense to add anything new via png.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extracted from iOS, so the only option available was PNG. I'm going to try to manually convert this.

Copy link
Member

Choose a reason for hiding this comment

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

Or ask design team for a help

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The results were low quality, I'll ask the design team for some help.

@SergeyZhukovsky
Copy link
Member

I believe the primary task with on-ramp implementation is to expand the support by adding additional Ramp networks(Sardine etc). The UI is good to have but it was previously required due to multiple options: ramp and wyre. As of now, it's just an extra step to select the only one option available. Isn't it better to keep the previous implementation to use ramp as a default option for buy? Is there any plan to add more network options to this screen? cc: @jamesmudgett @SergeyZhukovsky

I would say there are plans to add more, but @jamesmudgett would know better.

@simoarpe
Copy link
Collaborator Author

@Pavneet-Sing, here is a recording of the iOS flow that I copied. Compiled a debug version from the latest changes (Pulled the latest changes and recompiled right now just to be extra sure). The only difference seems to be this extra screen. Tried on both Solana and Ethereum networks. Let me know I'm missing something obvious.

Screen.Recording.2023-01-26.at.2.41.49.PM.mov

@simoarpe
Copy link
Collaborator Author

Update: I've just had a call with @jamesmudgett discussing the changes required. I'm going to ping you again once this PR is ready to review.

@simoarpe simoarpe removed the request for review from Pavneet-Sing January 26, 2023 15:57
@Pavneet-Sing
Copy link
Contributor

@Pavneet-Sing, here is a recording of the iOS flow that I copied. Compiled a debug version from the latest changes (Pulled the latest changes and recompiled right now just to be extra sure). The only difference seems to be this extra screen. Tried on both Solana and Ethereum networks. Let me know I'm missing something obvious.

Screen.Recording.2023-01-26.at.2.41.49.PM.mov

IOS sardine support: https://github.com/brave/brave-ios/blob/development/Sources/BraveWallet/Crypto/Stores/BuyTokenStore.swift#L24 but it is for US only: https://github.com/brave/brave-ios/blob/development/Sources/BraveWallet/Crypto/Stores/BuyTokenStore.swift#L160

Additionally, Desktop has Transak as well

Screenshot from 2023-01-26 21-53-12

@simoarpe simoarpe force-pushed the simone/on-ramp branch 2 times, most recently from 9d65975 to e8a253d Compare January 31, 2023 11:45
@simoarpe simoarpe changed the title Add purchase method screen with Ramp.Network Add purchase method screen with updated on-ramp methods (Ramp.Network, Sardine, Transak) Jan 31, 2023
@simoarpe
Copy link
Collaborator Author

Review feedback addressed @SergeyZhukovsky.

Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

@Pavneet-Sing
Copy link
Contributor

I am noticing a small delay to show Sardine option that can be displayed instantly. Just create a method to check if sardine is enabled and show it instantly. This will also simplify getBuyUrl:

public void getBuyUrl(int onRampProvider, String chainId, String from, String rampNetworkSymbol,
String amount, Resources resources, OnRampCallback callback) {
if (onRampProvider == OnRampProvider.SARDINE) {
// Sardine services are available only for US locales.
Locale currentLocale =
ConfigurationCompat.getLocales(resources.getConfiguration()).get(0);
if (currentLocale == null || !currentLocale.getCountry().equals("US")) {
callback.OnUrlReady(null);
return;
}
}

sardine-delay.webm

@simoarpe simoarpe merged commit 4379dd4 into master Feb 11, 2023
@simoarpe simoarpe deleted the simone/on-ramp branch February 11, 2023 12:04
@github-actions github-actions bot added this to the 1.50.x - Nightly milestone Feb 11, 2023
@@ -241,11 +241,11 @@ public void setWalletCoinAdapterType(AdapterType type) {
}

public void setWalletListItemModelList(List<WalletListItemModel> walletListItemModelList) {
this.walletListItemModelList = walletListItemModelList;
this.walletListItemModelList = removeDuplicates(walletListItemModelList);
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed to notice this before but this should be done where the data is fetched or processed then passed the filtered data to UI. Ok for now as a workaround, probably address this in the next PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is causing an issue to remove tokens while showing tokens from all networks so logged an issue brave/brave-browser#28506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS
Projects
None yet
4 participants