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

fix(eventindexer): nft balance changes should be wrapped in db transaction #16943

Merged
merged 7 commits into from
May 6, 2024

Conversation

the-laziest
Copy link
Contributor

@the-laziest the-laziest commented May 1, 2024

Increase and subtract of nft balance in db should be done in transaction to prevent corrupted data in case of error in second query

@the-laziest the-laziest changed the title fix(eventindexer): nft balance changes should be in db transaction fix(eventindexer): nft balance changes should be wrapped in db transaction May 1, 2024
Copy link
Contributor

@xiaodino xiaodino left a comment

Choose a reason for hiding this comment

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

@the-laziest Could you explain more what the PR is for and a way to reproduce the issue?

@the-laziest
Copy link
Contributor Author

@the-laziest Could you explain more what the PR is for and a way to reproduce the issue?

Sure, @xiaodino, if there will be an error for some reason here (related to db connection or smth like that), then in database NFT balance will be increased for To address, but not decreased for From address

@the-laziest the-laziest requested a review from xiaodino May 1, 2024 18:38
@cyberhorsey
Copy link
Contributor

cyberhorsey commented May 1, 2024

Three notes:

  1. I'm happy to merge this PR, however, we will not use any of this code, and it will be deleted for mainnet (since we will use professional NFT indexing APIs that already exist), so it is not robust on purpose.

  2. We can delete the existing IncreaseBalance and SubtractBalance methods now, since they are unused, so if this PR is to be merged, we should do that.

  3. gofmt has not been run on this code, and thus, it is failing basic formatting, as well as it is not passing our general linter warnings for golangcilint.

@the-laziest
Copy link
Contributor Author

@cyberhorsey, updated all mentioned things

packages/eventindexer/indexer/index_nft_transfers.go Outdated Show resolved Hide resolved
packages/eventindexer/nft_balance.go Outdated Show resolved Hide resolved
@cyberhorsey
Copy link
Contributor

Left two comments.

Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 32.14286% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 17.87%. Comparing base (f53766b) to head (ef07bba).
Report is 417 commits behind head on main.

Files Patch % Lines
...ckages/eventindexer/indexer/index_nft_transfers.go 0.00% 12 Missing ⚠️
packages/eventindexer/pkg/repo/nft_balance.go 56.25% 2 Missing and 5 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16943      +/-   ##
==========================================
- Coverage   26.82%   17.87%   -8.96%     
==========================================
  Files         105       45      -60     
  Lines        6188     1992    -4196     
==========================================
- Hits         1660      356    -1304     
+ Misses       4348     1593    -2755     
+ Partials      180       43     -137     
Flag Coverage Δ
eventindexer 17.87% <32.14%> (+0.64%) ⬆️
guardian-prover-health-check ?
relayer ?

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@the-laziest
Copy link
Contributor Author

the-laziest commented May 3, 2024

@cyberhorsey , could you please take a look at it?

@cyberhorsey
Copy link
Contributor

LGTM. Thanks for doing the requested changes.

@cyberhorsey cyberhorsey dismissed xiaodino’s stale review May 6, 2024 15:30

has been answered

@cyberhorsey cyberhorsey added this pull request to the merge queue May 6, 2024
Merged via the queue into taikoxyz:main with commit b134161 May 6, 2024
5 of 6 checks passed
@the-laziest
Copy link
Contributor Author

@cyberhorsey , can I please ask for contributor poap, if it's applicable to this pr

@RogerLamTd
Copy link
Contributor

@gitpoap-bot please give @the-laziest a poap

Copy link

gitpoap-bot bot commented May 6, 2024

Congrats, @the-laziest ! You've earned a GitPOAP for your contribution!

GitPOAP: 2024 Taiko Contributor:

GitPOAP: 2024 Taiko Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants