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

Conversation

@schottra
Copy link
Contributor

@schottra schottra commented Aug 1, 2023

Description

This PR creates a base set of buy-usdc sagas and layers a basic purchase-content slice on top of it. The goal was to make the usdc flow independent in case we need to call it from any other flows later on.

I also refactored some of the audius-backend logic used for userbanks to make it generic to both the audio and usdc cases, and moved all of that code into a solana service (instead of waudio).

The usdc and purchase flows are in common so that they can be used from both platforms, and some of the cleanup needed to make this possible caused the PR to be a little larger than I would like :-(

Dragons

I looked for all usages of the functions from services/waudio and updated them. But it's possible I missed some.

How Has This Been Tested?

Verified locally in Chrome

How will this change be monitored?

Will do a QA pass in staging to make sure we didn't break any userbank-related functionality

Feature Flags

Code that invokes this flow is behind the USDC feature flag.

schottra added 9 commits July 28, 2023 16:41
* origin/main:
  Update userbank function usage to pass config object (#3823)
  Update dapp-store build artifacts
  [C-2857] Revert remove get blocknumber (#3802)" (#3826)
  [C-2742] Multi-track form pagination (#3818)
  Bump mobile versions for client v1.5.35 full app release (#3827)
  Revert "Add purchased + reposted tracks to library PAY-1633  (#3820)" (#3825)
  Add purchased + reposted tracks to library PAY-1633  (#3820)
  Update SDK version + ActivityFull type (#3819)
  Use audius-query in USDC Purchase Drawer (#3822)
  Update bootstrap nodes (#3821)
  [PAY-1589] Wire up Stripe Onramp in mobile (#3814)
  v1.5.35
  Add favorite test and fix aria-label (#3817)
  [C-2908 C-2744] fix desktop follow button (#3816)
@gitguardian
Copy link

gitguardian bot commented Aug 1, 2023

⚠️ GitGuardian has uncovered 7 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
2858198 Generic High Entropy Secret 66abc9f packages/mobile/.env.prod View secret
2858199 Generic High Entropy Secret 66abc9f packages/mobile/.env.stage View secret
688750 Generic High Entropy Secret f58b8fe packages/web/.env/.env.stage View secret
2416684 Generic High Entropy Secret f58b8fe packages/web/.env/.env.stage View secret
688750 Generic High Entropy Secret 63525be packages/web/.env/.env.stage View secret
2416684 Generic High Entropy Secret 63525be packages/web/.env/.env.stage View secret
1606949 Generic High Entropy Secret cf3e4a4 packages/web/.env/.env.prod 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!

@dharit-tan dharit-tan self-assigned this Aug 1, 2023
@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/pay-1630-purchase-sagas

Copy link
Contributor

@rickyrombo rickyrombo left a comment

Choose a reason for hiding this comment

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

Looking real good! mostly nits. this is really coming together 🚀

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/pay-1630-purchase-sagas

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/pay-1630-purchase-sagas

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/pay-1630-purchase-sagas

@audius-infra
Copy link
Collaborator

It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype

@audius-infra
Copy link
Collaborator

It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype

@audius-infra
Copy link
Collaborator

It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype

@schottra schottra requested a review from dharit-tan August 9, 2023 20:41
@schottra
Copy link
Contributor Author

schottra commented Aug 9, 2023

@rickyrombo @dharit-tan If you don't mind taking another look (maybe just commits added since last review? or these 5 commits specifically.
I think this is ready to go.

eventName: Name.CREATE_USER_BANK_SUCCESS,
properties: { mint, recipientEthAddress }
})
return res.userbank
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rickyrombo I updated this to correctly return the user bank from the inner function!
Also reorganized a bit to make it less complicated and it will actually throw its error now

Copy link
Contributor

Choose a reason for hiding this comment

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

wanna make sure tipping works on stage after this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested it locally and it worked fine. But we definitely should re-verify after merging and during RC

@audius-infra
Copy link
Collaborator

It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype

* origin/main:
  [C-2930] Fix extra space after username in tip to unlock modal (#3845)
  QA-588 Fix collection card profile link  (#3853)
  Fix broken playlist fetch via resolve (#3863)
  [PAY-1695] DMs: Entrypoint Analytics (#3862)
  Minor improvements to SEO flow merged in #3859 (#3861)
@audius-infra
Copy link
Collaborator

It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype

@socket-security
Copy link

socket-security bot commented Aug 9, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

@@ -1,3 +1,3 @@
{
"name": "@audius/common",
"version": "1.5.36",
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't want this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you might have reviewed this while I was pushing the commit to update to 3.0.3. I think we want that version.

@@ -1,3 +1,3 @@
{
"name": "embed",
"version": "1.5.36",
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should also be fixed

} catch (err: any) {
// Catching error here for analytics purposes
const errorMessage = 'error' in err ? err.error : (err as any).toString()
const errorCode = 'errorCode' in err ? err.errorCode : undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

this would be a solana error code i imagine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a few error cases that can lead here:

  • TransactionHandler in libs gets an error back from relay with a http status code (usually 500). That would be this error code
  • An unexpected error while making the http request to relay
  • A runtime error of some sort (unexpected empty object, etc).

eventName: Name.CREATE_USER_BANK_SUCCESS,
properties: { mint, recipientEthAddress }
})
return res.userbank
Copy link
Contributor

Choose a reason for hiding this comment

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

wanna make sure tipping works on stage after this!

}
})
console.error(`Failed to create userbank, with err: ${err}`)
throw new Error(`Failed to create user bank: ${errorMessage}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

sure we want to throw here? would that brick the app if we forget to catch it downstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I debated this. We only use this code in a few places. In all of them, any of the failures that would lead here would put us in a state where continuing isn't feasible (the userbank doesn't exist, so we can't move on with the saga that wants to use it).

I'm happy to remove this for now, but I think it will result in strange behavior at best where calling code isn't handling these error cases and we end up throwing in a different place further down the line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @dylanjeffers @rickyrombo @sliptype Do we have any conventions around error handling in sagas? Is it more consistent here to just return nothing and let calling code detect the absence of a value? Or should we throw errors and let calling code try/catch?

Copy link
Contributor

Choose a reason for hiding this comment

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

my pattern in sagas is to try/catch things that throw and then report the error to sentry and put an error redux action

@dharit-tan
Copy link
Contributor

what's up with ESlint missing in the CI job?

* origin/main:
  [C-2926] Implement selected values for upload contextual menu fields (#3848)
  Preserve CIDs for track and collection cover arts (#3866)
@schottra
Copy link
Contributor Author

what's up with ESlint missing in the CI job?

@dharit-tan Not sure. Might be a package caching issue? I'll make sure it's working before I merge.
I did at least verify locally that there are no lint or type errors

@audius-infra
Copy link
Collaborator

It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype

@audius-infra
Copy link
Collaborator

It looks like there may be some changes to native mobile code, which requires triggering a full app release. Please follow the instructions here: https://www.notion.so/audiusproject/When-to-bump-app-version-2644a8f772364a4d91f44abcba44ce0b?pvs=4. cc @nicoback2 @sliptype

@schottra
Copy link
Contributor Author

@SocketSecurity ignore underscore@1.9.1
@SocketSecurity ignore lodash.template@4.2.4
@SocketSecurity ignore merge-deep@3.0.2

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/pay-1630-purchase-sagas

Copy link
Contributor

@rickyrombo rickyrombo left a comment

Choose a reason for hiding this comment

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

Woo! Huge PR, awesome work!

}
})
console.error(`Failed to create userbank, with err: ${err}`)
throw new Error(`Failed to create user bank: ${errorMessage}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

my pattern in sagas is to try/catch things that throw and then report the error to sentry and put an error redux action

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/pay-1630-purchase-sagas

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/pay-1630-purchase-sagas

@schottra schottra merged commit 15f056c into main Aug 10, 2023
@schottra schottra deleted the pay-1630-purchase-sagas branch August 10, 2023 20:53
audius-infra pushed a commit that referenced this pull request Aug 12, 2023
[3436c20] [PAY-1701] Fix "Share to DMs" to work through InboxUnavailableModal (#3874) Marcus Pasell
[a740243] Add sdk:update-hotfix (#3875) Dylan Jeffers
[a25fd19] [C-2759] Make donation link external (#3872) Dylan Jeffers
[15f056c] [PAY-1630] Wire up purchase content sagas (#3834) Randy Schott
[998d44b] Fix mobile crash on drawer dismiss (#3871) Reed
[7d0e0b3] [PAY-1430] "Leaving Audius" Warning for External Links (and createModal helper) (#3860) Marcus Pasell
[bee8bd1] Remove .only on upload cypress test (#3869) Raymond Jacobson
[4c0b25f] [C-2926] Implement selected values for upload contextual menu fields (#3848) Dylan Jeffers
[5773578] Preserve CIDs for track and collection cover arts (#3866) Marcus Pasell
[be0d278] [C-2930] Fix extra space after username in tip to unlock modal (#3845) nicoback2
[f5320be] QA-588 Fix collection card profile link  (#3853) nicoback2
[360416e] Fix broken playlist fetch via resolve (#3863) Raymond Jacobson
[2dc2c29] [PAY-1695] DMs: Entrypoint Analytics (#3862) Marcus Pasell
[f80d366] Minor improvements to SEO flow merged in #3859 (#3861) Raymond Jacobson
[b99d62f] Add nodes to env for SEO support (#3859) Raymond Jacobson
[20476ee] [C-2941] Modify cloudflare worker to pull in SEO data from discovery nodes (#3858) Raymond Jacobson
[7f79830] [C-2879] Add validation to single track upload flow (#3855) Kyle Shanks
[6f4fc89] [C-2940] Update google analytics tags and fix embed build (#3856) Raymond Jacobson
[3469c89] [C-2852 PLAT-1094 PLAT-1093] Add fetch collection by permalink (#3751) Dylan Jeffers
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
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.

6 participants