Skip to content

[QA-1491] Fix balance polling bug #9402

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.

Sign up for GitHub

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

Jump to bottom
Merged
raymondjacobson merged 2 commits into from
Aug 9, 2024
Merged

[QA-1491] Fix balance polling bug #9402

raymondjacobson merged 2 commits into from
Aug 9, 2024

Conversation

@raymondjacobson
Copy link
Member

@raymondjacobson raymondjacobson commented Aug 8, 2024 •
edited
Loading

Description

Consider the following scenario:

  1. I go to check out with 0 $USDC balance
  2. I click "Transfer via crypto"
  3. I send 1 $USDC from my external wallet to my token account using processed commitment level (I can't control this, phantom, etc. would)
  4. The client polls for balance using processed commitment on the FE RPC endpoint and sees that we have 1 $USDC now
  5. I click buy and my tx fails because the RPC that relays the tx doesn't know the balance yet

To fix, introduce a commitment level from the audius-fe perspective. This should dramatically dramatically reduce the probability of this happening. Yes this does come at the cost of your balance not showing up as fast, but that's also probably the right call versus failed tx.

How Has This Been Tested?

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

Purchased some stuff on stage

@changeset-bot
Copy link

changeset-bot bot commented Aug 8, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: e51b377

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@audius-infra
Copy link
Collaborator

audius-infra commented Aug 8, 2024

Preview this change https://demo.audius.co/rj-qa-1491

schottra
schottra approved these changes Aug 8, 2024
View reviewed changes
Copy link
Contributor

@schottra schottra left a comment

Choose a reason for hiding this comment

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

lgtm, though I wonder if the different levels of commitment in various parts of the app might cause confusion. Should we not just update all of them to use 'confirmed'?

packages/common/src/services/audius-backend/solana.ts Outdated
tokenAccount,
mint = DEFAULT_MINT,
tokenAccount
commitment
Copy link
Contributor

@schottra schottra Aug 8, 2024

Choose a reason for hiding this comment

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

You used a default in the hook, but not here? Seems like it should be consistent.

Copy link
Member Author

@raymondjacobson raymondjacobson Aug 9, 2024

Choose a reason for hiding this comment

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

ye

@raymondjacobson
Copy link
Member Author

raymondjacobson commented Aug 9, 2024

lgtm, though I wonder if the different levels of commitment in various parts of the app might cause confusion. Should we not just update all of them to use 'confirmed'?

This is a good point. I think it's ok to used processed sometimes. There are experiences where a user may want to see their balance getting updated real-time with purchases, but not have the expectation of immediate withdrawal. and it is nice how fast it reflects after a sale. I want to think about this more, but will ship to fix the obvious bug for now.

@raymondjacobson raymondjacobson enabled auto-merge (squash) August 9, 2024 01:49
@audius-infra
Copy link
Collaborator

audius-infra commented Aug 9, 2024

Preview this change https://demo.audius.co/rj-qa-1491

@raymondjacobson raymondjacobson merged commit f2eabb1 into main Aug 9, 2024
@raymondjacobson raymondjacobson deleted the rj-qa-1491 branch August 9, 2024 02:01
@rickyrombo
Copy link
Contributor

rickyrombo commented Aug 9, 2024

Will check more in depth tomorrow on this particular case but I think we can avoid a lot of these issues by setting the preflightCommitment rather than making everything slower potentially

audius-infra pushed a commit that referenced this pull request Aug 10, 2024
Audius Client (Web and Mobile) v1.5.94
2f2ff17 
[7121301] Set is_custom_musical_key (#9418) Sebastian Klingler
[ee99fd4] add is_custom_musical_key (#9416) Michelle Brier
[f30a637] [PAY-3332] Remixers users endpoint (#9401) Reed
[f2eabb1] [QA-1491] Fix balance polling bug (#9402) Raymond Jacobson
[c5e7348] Fix lint (#9412) Raymond Jacobson
[5598656] [QA-1356] Improve notification UI (#9390) Raymond Jacobson
[dcb74eb] Fix useClickOutside (#9409) Andrew Mendelsohn
[975b630] Add better error messages for Claimable Tokens Program (#9369) Marcus Pasell
[17904fb] [PAY-3335] Add priority fees to SDK Solana usages (#9399) Marcus Pasell
[c34cd83] Record Track Downloads via Entity Manager (#9103) Marcus Pasell
[e59dfc8] [QA-1357] Remove excessive polling and amplitude events in unlock (#9391) Raymond Jacobson
[26b2fc9] [QA-1496] fix unsafe read of audio analysis field (#9406) Randy Schott
[abc1b37] [C-4934] Search bar fixes (#9403) Sebastian Klingler
[d525a1b] [C-4930, C-4931] Add IconText, ArtistPick, and Identifier components to harmony and harmony native (#9400) Kyle Shanks
[53bf3e2] [PAY-3321][PAY-3333] Add network cut feature flag and update staking bridge and payment router addresses (#9393) Saliou Diallo
[4d0fbc2] [PAY-3294] Hide save/repost for locked albums on mobile (#9333) Reed
[7e3a75c] [C-4936] FilterButton fixes (#9397) Sebastian Klingler
[517251d] [PAY-3286] Upload/edit confirmation drawer & modal rework (#9326) Raymond Jacobson
[dce9903] [C-4920] Only show bpm decimals if necessary (#9398) Sebastian Klingler
[d6dbab0] [C-4937] Adding some pagination buttons (#9394) JD Francis
[f183f7e] [PAY-3336] fix isPlaying logic for details screens (#9395) Randy Schott
[5ed523f] Add comment actions: edit, delete, react (#9372) Isaac Solo
[88e24b5] [C 4922] Update mobile filter button (#9385) Dylan Jeffers
[901d811] [C-4923] Persist verified filter & general cleanup (#9384) Sebastian Klingler
[bab49d6] Add Pin icon and Timestamp component to harmony and harmony-native (#9383) Kyle Shanks
[53a1a32] [QA-1485] Redir to signup/in on direct /feed link (#9373) Raymond Jacobson
[c618eb9] Improve useClickOutside (#9377) Sebastian Klingler
[a97deb8] [PAY-2267] Update stage payment router and program addresses (#9387) Raymond Jacobson
[def034f] [QA-1446] Fix popup alignment (#9389) Raymond Jacobson
[c86bc70] Add priority fees to purchases (#9392) Marcus Pasell
[66f1a0e] [PAY-3314] Track select dropdown for purchase/remix targeting (#9386) Andrew Mendelsohn
[5fe7681] Comments Read/Create E2E - Changes to DB/Discovery/SDK/Desktop UI (#9319) JD Francis
[1ccfcd4] [PAY-3331] Purchase/sale endpoint filtering by content id (#9376) Reed
[fc59a10] [PAY-3330] Fix spacing on RepostsFavoritesStats (#9375) Raymond Jacobson
[7712005] Fix short trending bug (#9378) Raymond Jacobson
[196fdba] [QA-1114] Include menu items even if buttons are present (#9381) Raymond Jacobson
[8e6b1e8] [QA-1486] Fix sign up select artists screen (#9371) Sebastian Klingler
[34d4417] [PAY-3313] Targeted DMs CTA and modal (#9364) Andrew Mendelsohn
[3a19003] [C-4921] Fix search v2 navigating back from profile (#9370) Sebastian Klingler
[d266394] [C-4918] Improve harmony web FilterButton (#9366) Dylan Jeffers
[4ef81a7] Bump mobile versions to 110 (#9367) Dylan Jeffers
schottra added a commit that referenced this pull request Aug 13, 2024
@schottra
Merge remote-tracking branch 'origin/main' into pay-2917-user-endpoin…
a8b38f4 
…ts-round-2

* origin/main: (43 commits)
  Testnet configuration for cometbft core (#9421)
  Fix pagination args (#9425)
  [QA-1501] Fix mobile search input (#9426)
  Add paginations to comments API (#9407)
  [C-4932] Add CommentBody component to harmony and harmony-native (#9415)
  Audius Client (Web and Mobile) v1.5.94
  check is_custom_musical_key before pulling in calculated key in repairer (#9417)
  Set is_custom_musical_key (#9418)
  add is_custom_musical_key (#9416)
  fix repairer eager loading (#9414)
  [PAY-3332] Remixers users endpoint (#9401)
  clean up aa endpts and limit ffprobe fields (#9413)
  relay retries (#9410)
  Audius Protocol v0.6.166
  [QA-1459] Fix 0087_fix_slugs_with_slash.sql migration (#9411)
  [QA-1491] Fix balance polling bug (#9402)
  Fix lint (#9412)
  [QA-1356] Improve notification UI (#9390)
  Fix useClickOutside (#9409)
  Add better error messages for Claimable Tokens Program (#9369)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@schottra schottra schottra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@raymondjacobson @audius-infra @rickyrombo @schottra