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

USDC Withdrawal saga scaffolding #3926

Merged
merged 2 commits into from
Aug 22, 2023
Merged

USDC Withdrawal saga scaffolding #3926

merged 2 commits into from
Aug 22, 2023

Conversation

dharit-tan
Copy link
Contributor

@dharit-tan dharit-tan commented Aug 22, 2023

Description

Scaffolding for USDC withdrawal saga!

Copied solana address validation logic from SendInputBody.tsx. I could spend time trying to convert that to use this fn in common, but it's an async fn and i don't wanna get distracted.

Dragons

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

How Has This Been Tested?

Dispatching actions via redux extension on local web.

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.

@audius-infra
Copy link
Collaborator

Preview this change https://demo.audius.co/rt-withdraw-saga

}
try {
// @ts-ignore - need an unused variable to check if the destinationWallet is valid
const ignored = new solanaweb3.PublicKey(destinationWallet)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we call new PublicKey() without setting it to a variable?

Also, is this a wallet or associated token account? Because we isOnCurve can help us check if it's a "root" wallet address

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 we want to support both. probably don't need to differentiate between them until we actually begin the withdrawal flow. good to know tho, will probably need to use isOnCurve for that step.
will try calling ctr without assigning to var.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without assigning ctr to var, i get this eslint error: Do not use 'new' for side effects. internet suggests this is the way. gonna leave for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. I do think we should avoid // @ts-ignore if at all possible, as it might hide other errors

Copy link
Contributor

Choose a reason for hiding this comment

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

Could always just boolean-ify the variable:
return !!pubkey

name: 'withdraw-usdc',
initialState,
reducers: {
setDestinationAddress: (
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I don't think we need action/reducer/sagas for setting destination/amount. It might be nice to keep the component responsible for the validations (or use hooks), and send an action to the store only once they're valid and when we need to start the complex transaction logic.

So much of the "saga-first" approach is for the shared code win with mobile and some useful debugging things, but I don't think we're going to have this on mobile anyway so it might be nice to forgo the saga indirection and keep things more closely colocated.

It would also help prevent stale data in the redux store and removes the need for a special "cleanup" action, as there could be a single action payload that's all used in one saga vs properties set on the redux store state piecemeal that then are used in the saga.

Just something to consider, moreso for future - don't have to change this now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice good thinking. i was kinda just following patterns but yeah you're probs right.

@dharit-tan dharit-tan merged commit 5d01710 into main Aug 22, 2023
2 checks passed
@dharit-tan dharit-tan deleted the rt-withdraw-saga branch August 22, 2023 20:31
schottra added a commit that referenced this pull request Aug 25, 2023
* origin/main:
  Add lint check for console.log and remove some bad ones (#3930)
  [C-2968] Fix private collection action buttons (#3937)
  Fix canonical url consistency (#3938)
  [C-2689] Add upload confirmation modal (#3934)
  [C-2966] Make sure that collection description limits are set to 1000 (#3935)
  Move sitemap hostname back to audius.co (#3931)
  Client uses cids in requests to CN for images (#3882)
  Add library albums and playlists audius-query hook + migrate collection reformat util; bump SDK PAY-1679 (#3864)
  [C-2982] Fix seo based on ahref recommendations (#3929)
  Migrate withdraw USDC saga to web common (#3928)
  USDC Withdrawal saga scaffolding (#3926)
  Fix useAllPaginated query C-2980 (#3924)
  Fix infinite scrolling cards C-2979 (#3923)
  [PAY-1632] Clean up and improve performance of music confetti (#3921)
  Revert "Update twitter icon on mobile (#3880)" (#3925)
audius-infra pushed a commit that referenced this pull request Aug 26, 2023
[b39cb5d] [PAY-1733] Remove Gated Prompt Modal (#3948) Marcus Pasell
[a522294] [PAY-1748][PAY-1731][PAY-1729][PAY-1730] DMs link fixes (#3946) Marcus Pasell
[adea357] quick linting fix (#3945) Kyle Shanks
[a0ff27c] Add embed cloudflare deployment and CI (#3940) Raymond Jacobson
[337f80f] [PAY-1727] USDC Withdrawals saga pt. 1 (#3932) Reed
[5bf820c] [C-2956] Add new Access & Sale modal to legacy upload form (#3900) Andrew Mendelsohn
[5258675] [C-2986] Upload flow qa round 1 (#3941) Kyle Shanks
[dd30c09] Use does_current_user_subscribe API field (#3943) Michelle Brier
[778a518] [C-2987] Add UserGeneratedText (#3942) Dylan Jeffers
[bb9e0cf] Update pull_request_template.md (#3939) Raymond Jacobson
[930dd1a] [C-2977] Fix collection page seo (#3936) Dylan Jeffers
[1e50b85] Update README.md (#3911) sabrina-kiam
[687fd28] Add lint check for console.log and remove some bad ones (#3930) Raymond Jacobson
[9f67e7b] [C-2968] Fix private collection action buttons (#3937) Dylan Jeffers
[7ab7a38] Fix canonical url consistency (#3938) Dylan Jeffers
[b1fbef8] [C-2689] Add upload confirmation modal (#3934) Kyle Shanks
[eb41fd3] [C-2966] Make sure that collection description limits are set to 1000 (#3935) Kyle Shanks
[6258484] Move sitemap hostname back to audius.co (#3931) Raymond Jacobson
[1e0c335] Client uses cids in requests to CN for images (#3882) Michelle Brier
[7974acc] Add library albums and playlists audius-query hook + migrate collection reformat util; bump SDK PAY-1679 (#3864) nicoback2
[7cba35b] [C-2982] Fix seo based on ahref recommendations (#3929) Dylan Jeffers
[05744b3] Migrate withdraw USDC saga to web common (#3928) Reed
[5d01710] USDC Withdrawal saga scaffolding (#3926) Reed
[4e0c480] Fix useAllPaginated query C-2980 (#3924) nicoback2
[21eb35c] Fix infinite scrolling cards C-2979 (#3923) nicoback2
[0025261] [PAY-1632] Clean up and improve performance of music confetti (#3921) Raymond Jacobson
[7ed2248] Revert "Update twitter icon on mobile (#3880)" (#3925) Reed
[1091934] [PAY-1742] Remove useMetaMask on invalid account (#3920) Raymond Jacobson
[6e303b1] [PAY-1741] Add routes for transactional pages (#3916) Randy Schott
[74cdb3c] Remove ontouchstart from index.html (#3919) Raymond Jacobson
[1a0332c] Improve lighthouse score (#3918) Raymond Jacobson
[a63896f] [PAY-1706] Merge modalsWithState with modals in common store (#3908) Marcus Pasell
[6a08944] [C-2976] Fix profile-page seo (#3912) Dylan Jeffers
[2381d46] Fix account details css (#3917) Raymond Jacobson
[8c24bdd] Fix mobile share of playlist permalink (#3913) sabrina-kiam
[1e376fe] [C-2911] Update new select page of the upload flow (#3910) Kyle Shanks
[bc0226f] Fix stripe modal opening behavior (#3914) Raymond Jacobson
[a3219c3] [C-2975] Fix stale local data (#3915) Dylan Jeffers
[748fdfd] PAY-1724 Add color specialGreen on mobile (#3909) Reed
[5c59fe5] [PAY-1628] Navigate to track after purchase (#3904) Randy Schott
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.

4 participants