Skip to content
This repository was archived by the owner on Oct 4, 2023. It is now read-only.

Add sagas for OnRamp to AUDIO#1666

Merged
rickyrombo merged 24 commits into
mainfrom
mjp-cb-onramp-sagas-v2
Aug 12, 2022
Merged

Add sagas for OnRamp to AUDIO#1666
rickyrombo merged 24 commits into
mainfrom
mjp-cb-onramp-sagas-v2

Conversation

@rickyrombo

@rickyrombo rickyrombo commented Aug 2, 2022

Copy link
Copy Markdown
Contributor

Description

Supercedes:

Main updates:

  • Quotes and exchanges aren't directly done via actions but instead are replaced by specific routines. Instead separate quote actions for USDC and SOL, there is one action that provides purchaseInfo, strictly information regarding the purchase of SOL for a SOL => AUDIO transaction with an estimate for how much that costs in USD (using a quote from that amount of SOL to USD.
    • This allows for us to build in padding into the quote for:
      • Transaction Fees: we call the computeRoutes on Jupiter to get the swap transactions, then compile them and get associated fees via Solana's API (this is generally a constant per transaction of 0.000005 SOL, although the Jupiter transactions can vary from 1-3 depending on how large the swap transaction gets)
      • Associated Token Account (ATA) rent exemption fees: Once we have the route from Jupiter, we also have the intermediate tokens needed for the swap. We check for an existing ATA on the user's wallet for each token, and if not present, we add the rent exemption minimum (calculated by calling the Solana method with 165 bytes) to the quote to cover that fee.
      • Slippage: To help ensure that the user does not end up with less $AUDIO than the whole number they specified, we preemptively pad the quote such that if the slippage were to bottom out (ie if slippage tolerance is set to 3% and the price slips 3%), the user would still get the expected amount of $AUDIO.
        • Slight dragon here: The quote used to generate the amount of SOL to purchase is different than the quote used to exchange that SOL for AUDIO. The price could drop more than the slippage tolerance due to this and still successfully exchange (since the first quote is separate from the second, and the second quote might have a lower AUDIO amount than the first), and the user might end up with less AUDIO. In practice, this is unlikely with a high slippage tolerance + this adjustment. A good side-effect from this is that users won't see any swap failures due to slippage, which is far worse than receiving less AUDIO.
  • Likewise, instead of an action for directly executing an exchange, the exchange is wrapped up in a saga that gets triggered on an OnRamp completion.
  • I added polling on wallet balances and extracted some of the pieces to the BuyAudio.ts audius-backend file
  • There's now a much more easily followed BuyAudioStage enum that indicates where in the flow we are (eg. are we waiting on the user to purchase SOL? For coinbase pay's funds to show up in the wallet? For the swap to complete?)

Dragons

Is there anything the reviewer should be on the lookout for? Are there any dangerous changes?

  • I used Solana's API to get the real fees and ATA rent exemption minimums. This means I can do so exactly requiring no buffer amount, but I still think I might add a slight buffer amount just to be sure we don't fail due to some arithmetic rounding error or something.
  • Speaking of arithmetic, there's a lot of different conversions happening - there's JSBI which is used by Jupiter, there's BN which is what we use, there's u64 which is what some Solana stuff uses now (a wrapper around BN that annoyingly is lowercased so doing new u64(..) triggers our linter), and there's number which is also used by Solana. I've mixed returning SOL and lamports in this PR to some degree... could use another pass on cleaning that up probably. Ideally I'd do everything in some sort of big int type to avoid any potential float precision stuff.
  • Not super dragony, but mildly infuriating: We have all sorts of different conversions in the wallet utils for different things, but they all have wildly different naming conventions and uses. These conversions could use some love...

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide repro instructions & any configuration.

Tested E2E using a prototype BuyAudio modal as well as just by triggering sagas manually and using a phantom wallet. I also closed all my ATAs and ran it again to see the ATA fee estimate get higher and lower depending on if it was my first or second time onramping. So far have not seen any "Insufficient funds" errors.

How will this change be monitored?

For features that are critical or could fail silently please describe the monitoring/alerting being added.

Feature Flags

Are all new features properly feature flagged? Describe added feature flags.

  • TODO: Add optimizely flags. (Although in the current state it's dead code)

@rickyrombo rickyrombo changed the base branch from main to mjp-cb-onramp-sagas August 4, 2022 19:17
@rickyrombo rickyrombo changed the base branch from mjp-cb-onramp-sagas to main August 10, 2022 20:18
@rickyrombo rickyrombo marked this pull request as ready for review August 10, 2022 21:33

@piazzatron piazzatron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Epic PR! Really just a few minor things - let's get this merged and test the heck out of it 🚀

state.feesCache = initialState.feesCache
},
restart: (state) => {
state.stage = BuyAudioStage.START

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to restart the purchaseInfoStatus with these stage transitions? If so, would modeling these as a discrimated union help?

DeletePlaylistConfirmation: false,
FeatureFlagOverride: false
FeatureFlagOverride: false,
BuyAudio: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason this is true by default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

testing purposes 😅 (shouldn't even be in here oops)

* Gets a quote from Jupiter for an exchange from inputTokenSymbol => outputTokenSymbol
* @returns the best quote including the RouteInfo
*/
function* getQuote({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe getQuote should take jup in as an argument to make this easier to test? Another approach would be to have a class encapsulating Jupiter and exposing this as a method on the class, but don't feel super strongly.

)

const estimatedLamports =
inSol + associatedAccountCreationFees + transactionFees - existingBalance

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Safe to do this not using BN/JSBI?

Comment on lines +33 to +43
export const getRootSolanaAccount = async () => {
await waitForLibsInit()
return libs().solanaWeb3Manager.solanaWeb3.Keypair.fromSeed(
libs().Account.hedgehog.wallet.getPrivateKey()
) as Keypair
}

export const getSolanaConnection = async () => {
await waitForLibsInit()
return libs().solanaWeb3Manager.connection as Connection
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few of these feel like general Solana utils rather than something that belongs specifically in BuyAudio.ts to me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah where should those live?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe /services/audius-backend/Solana.ts?

return tokenAccountInfo
}

export const pollForAudioBalanceChange = async ({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe include a comment that this throws (do we want it to throw?)

@gitguardian

gitguardian Bot commented Aug 11, 2022

Copy link
Copy Markdown

⚠️ GitGuardian has uncovered 17 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
688750 Generic High Entropy Secret ad9d272 packages/mobile/.env.dev.tmpl View secret
2416684 Generic High Entropy Secret ad9d272 packages/mobile/.env.dev.tmpl View secret
2416685 Generic High Entropy Secret ad9d272 packages/mobile/.env.dev.tmpl View secret
2416686 Generic High Entropy Secret ad9d272 packages/mobile/.env.dev.tmpl View secret
2858199 Generic High Entropy Secret ad9d272 packages/mobile/.env.dev.tmpl View secret
688750 Generic High Entropy Secret ad9d272 packages/mobile/.env.prod.tmpl View secret
2460749 Generic High Entropy Secret ad9d272 packages/mobile/.env.prod.tmpl View secret
2460750 Generic High Entropy Secret ad9d272 packages/mobile/.env.prod.tmpl View secret
2460751 Generic High Entropy Secret ad9d272 packages/mobile/.env.prod.tmpl View secret
1606949 Generic High Entropy Secret ad9d272 packages/mobile/.env.prod.tmpl View secret
2111319 Generic High Entropy Secret ad9d272 packages/mobile/.env.prod.tmpl View secret
2858198 Generic High Entropy Secret ad9d272 packages/mobile/.env.prod.tmpl View secret
688750 Generic High Entropy Secret ad9d272 packages/mobile/.env.stage.tmpl View secret
2416684 Generic High Entropy Secret ad9d272 packages/mobile/.env.stage.tmpl View secret
2416685 Generic High Entropy Secret ad9d272 packages/mobile/.env.stage.tmpl View secret
2416686 Generic High Entropy Secret ad9d272 packages/mobile/.env.stage.tmpl View secret
2858199 Generic High Entropy Secret ad9d272 packages/mobile/.env.stage.tmpl View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@audius-infra

Copy link
Copy Markdown
Collaborator

Preview this change https://demo.audius.co/mjp-cb-onramp-sagas-v2

@rickyrombo rickyrombo merged commit bcd68c7 into main Aug 12, 2022
@rickyrombo rickyrombo deleted the mjp-cb-onramp-sagas-v2 branch August 12, 2022 22:33
audius-infra pushed a commit that referenced this pull request Aug 13, 2022
[0d2f211] [C-826] Move remote-config-instance to saga context (#1713) Dylan Jeffers
[bcd68c7] Add sagas for OnRamp to AUDIO (#1666) Marcus Pasell
[1ca2af2] Prevent showing file explorer on pressing enter (#1704) Saliou Diallo
[0b6f0e4] [PAY-521] Add web dethroned notification (#1712) Michael Piazza
[76b1b0e] [C-820] Move apiClient into saga context, Add WalletClient to context (#1711) Dylan Jeffers
[2706cef] [C-807] Move audiusBackendInstance into context across all sagas (#1710) Dylan Jeffers
[25c9433] [PAY-466] Display AAO emoji errors for failed reward claims (#1693) Reed
[c2a79ff] [C-808] Don't show unnecessarily show profile option on overflow menu (#1708) Dylan Jeffers
[94e188f] [C-801] Add tag search text to mobile (#1706) Dylan Jeffers
[a47c2e2] [C-806] Prevent multiple overflow menus at a time (#1698) Saliou Diallo
[dce8a58] [C-797] Fix track-remix-screen text color (#1705) Dylan Jeffers
[1325829] Add extends null (#1707) Sebastian Klingler
[51e38d6] [C-805] Fix artifact on small card (#1696) Raymond Jacobson
[0a521b1] [C-798] Remove owned/created stamp from collectible tile (#1689) Sebastian Klingler
[18ae0a0] [C-804] Show playlist count for non-artists in native (#1697) Dylan Jeffers
[ffcf0da] Fix associated wallets (#1692) Raymond Jacobson
[e287fb1] [PAY-267, C-753] Remove tipping feature flags, Fix mutuals icon (#1695) Dylan Jeffers
[118102d] [C-794] Add typecheck to web ci (#1694) Sebastian Klingler
[4f60a49] [C-803] Add common state and backend sagas to native (#1691) Dylan Jeffers
[96d9cc8] [C-800] Add native saga context and update common sagas (#1690) Dylan Jeffers
[12add4a] [C-799] Add saga context, refactor backend state (#1688) Dylan Jeffers
[afdbe97] [C-796] Refactor AudiusAPIClient to work in native context (#1687) Dylan Jeffers
[a185c09] [C-795] Fix user-list tag imports (#1686) Dylan Jeffers
[3a8704c] [C-774] Move audius-backend to common, add native libs-instance (#1685) Dylan Jeffers
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants