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

feat(wallet)_: added supported chains and recurrent purchase url to o… #5231

Merged
merged 1 commit into from
May 28, 2024

Conversation

dlipicar
Copy link
Contributor

@dlipicar dlipicar commented May 24, 2024

…nramp providers

Closes status-im/status-desktop#14817

Moved onramp code to separate package for better organization.
Moved static onramp provider definitions to separate file.
Added supported chains (used in the mobile designs) and recurrent URL (to be used in status-im/status-desktop#14818) to onramp providers.

Tested on desktop, everything looks the same
image

@dlipicar dlipicar force-pushed the feat/additional-onramp-fields branch from 173d317 to e2b3abd Compare May 24, 2024 19:28
@dlipicar dlipicar marked this pull request as ready for review May 24, 2024 19:30
@status-im-auto
Copy link
Member

status-im-auto commented May 24, 2024

Jenkins Builds

Click to see older builds (5)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 173d317 #1 2024-05-24 19:31:09 ~7 min ios 📦zip
✔️ e2b3abd #2 2024-05-24 19:36:55 ~4 min ios 📦zip
✔️ e2b3abd #2 2024-05-24 19:38:02 ~4 min linux 📦zip
✔️ e2b3abd #2 2024-05-24 19:40:13 ~6 min android 📦aar
✖️ e2b3abd #2 2024-05-24 20:21:42 ~47 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ dacae39 #3 2024-05-28 13:46:09 ~3 min ios 📦zip
✔️ dacae39 #3 2024-05-28 13:46:59 ~4 min linux 📦zip
✔️ dacae39 #3 2024-05-28 13:48:49 ~6 min android 📦aar
✔️ dacae39 #3 2024-05-28 14:29:42 ~47 min tests 📄log

SiteURL string `json:"siteUrl"`
RecurrentSiteURL string `json:"recurrentSiteUrl"`
Hostname string `json:"hostname"`
Params map[string]string `json:"params"` // TODO implement params in JSON and parsing status-mobile
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the comment was not added by you, but if it's still relevant, can we move this to a GitHub issue? It will be easier to track.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as this I think... #5231 (comment)
This needs some cleanup/rework probably, but I want to change as little as possible until there's a real need.

Comment on lines +12 to +39
ramps := []CryptoOnRamp{
{
Name: "Ramp",
Description: "Global crypto to fiat flow",
Fees: "0.49% - 2.9%",
LogoURL: logoRamp,
SiteURL: "https://ramp.network/buy?hostApiKey=zrtf9u2uqebeyzcs37fu5857tktr3eg9w5tffove&swapAsset=DAI,ETH,USDC,USDT",
Hostname: "ramp.network",
SupportedChainIDs: []uint64{walletCommon.EthereumMainnet, walletCommon.ArbitrumMainnet, walletCommon.OptimismMainnet},
},
{
Name: "MoonPay",
Description: "The new standard for fiat to crypto",
Fees: "1% - 4.5%",
LogoURL: logoMoonPay,
SiteURL: "https://buy.moonpay.com/?apiKey=pk_live_YQC6CQPA5qqDu0unEwHJyAYQyeIqFGR",
Hostname: "moonpay.com",
SupportedChainIDs: []uint64{walletCommon.EthereumMainnet, walletCommon.ArbitrumMainnet, walletCommon.OptimismMainnet},
},
{
Name: "Latamex",
Description: "Easily buy crypto in Argentina, Mexico, and Brazil",
Fees: "1% - 1.7%",
LogoURL: logoLatamex,
SiteURL: "https://latamex.com/",
Hostname: "latamex.com",
SupportedChainIDs: []uint64{walletCommon.EthereumMainnet},
},
Copy link
Contributor

@ajayesivan ajayesivan May 27, 2024

Choose a reason for hiding this comment

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

I don't see any providers with RecurrentSiteURL. Don't we still have any recurrent options?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. We have a separate issue for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap! Mercuryo is coming as soon as we sign the deal! That will be our first recurrent-enabled provider, but I wanted to leave the API ready beforehand in case someone wants to start doing the client work.

Copy link
Contributor

@ajayesivan ajayesivan left a comment

Choose a reason for hiding this comment

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

LGTM! Tested on Mobile 👍

@ajayesivan
Copy link
Contributor

Added supported chains (used in the mobile designs)

I remember seeing the supported chains in the design, but I can't find them anymore. Figma Design. Do you have a link to that?

}

func (c *Manager) getFromHTTPDataSource() ([]CryptoOnRamp, error) {
if c.options.DataSource == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

dont we need this same check in getFromStaticDataSource?

@@ -61,8 +62,8 @@ func NewService(
feed *event.Feed,
mediaServer *server.MediaServer,
) *Service {
cryptoOnRampManager := NewCryptoOnRampManager(&CryptoOnRampOptions{
dataSourceType: DataSourceStatic,
cryptoOnRampManager := onramp.NewManager(&onramp.Options{
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like we are setting default to Static? is this a todo to also support fetch from http?
I am not sure I understand when we would do a static fetch and when we would perform a http request in case of on ramp providers?

Copy link
Contributor Author

@dlipicar dlipicar May 28, 2024

Choose a reason for hiding this comment

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

So... This code was done like 3 years ago and apparently the original spec was to be able to fetch the list of providers from the web (so we don't need to release a new Status version to change it). However, there were some issues with that approach, a static list was introduced and the http source was left as eternal tech debt :-)
This whole thing could use some cleanup/rework, but there's no real need for that at the moment. I just moved some code around for better organization, but otherwise tried changing as little as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the diff is messed up, there's almost no new code other than:

  • Moving the list of providers to a separate file
  • Specifying the static list of providers as structs instead of JSON strings
  • Adding chains and recurrentURL fields to struct

Copy link
Contributor

@Khushboo-dev-cpp Khushboo-dev-cpp left a comment

Choose a reason for hiding this comment

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

LGTM :)

@dlipicar
Copy link
Contributor Author

Added supported chains (used in the mobile designs)

I remember seeing the supported chains in the design, but I can't find them anymore. Figma Design. Do you have a link to that?

hmm it was there, but it looks like it got removed to match the desktop designs... I guess I could remove it, though it doesn't hurt to have the extra info.

@dlipicar dlipicar force-pushed the feat/additional-onramp-fields branch from e2b3abd to dacae39 Compare May 28, 2024 13:42
@dlipicar dlipicar merged commit 717c7df into develop May 28, 2024
12 checks passed
@dlipicar dlipicar deleted the feat/additional-onramp-fields branch May 28, 2024 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support recurring onramp providers
4 participants