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

Check for existing asset in db before attempting to insert a new asset. #687

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Nov 20, 2023

This PR is another solution for #665 .

In these commits we add a check to ensure that if a confirmed asset already exists in our db we do not attempt to insert another copy.

This solution is a better solution than #684 because it is simpler and less risky.

Copy link
Contributor

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Looks good! Confirmed that the itest does test this issue of displayed balance.

Sounds like another angle for a fix is to have the custodian not perform these duplicate proof fetches on restart in the first place?

tapdb/assets_common.go Show resolved Hide resolved
itest/tapd_harness.go Show resolved Hide resolved
Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

Lgtm

itest/send_test.go Show resolved Hide resolved
@ffranr
Copy link
Contributor Author

ffranr commented Nov 20, 2023

Sounds like another angle for a fix is to have the custodian not perform these duplicate proof fetches on restart in the first place?

@jharveyb yes, I would like to refactor the custodian in another PR. It doesn't handle the events cache very cleanly.

@ffranr ffranr added this pull request to the merge queue Nov 20, 2023
@Roasbeef
Copy link
Member

Sounds like another angle for a fix is to have the custodian not perform these duplicate proof fetches on restart in the first place?

Yep, this also needs to be resolved. We have some candidate fixes in mind, and can exercise the fixes at the unit test level.

Merged via the queue into main with commit d09b3bf Nov 20, 2023
27 of 28 checks passed
tapdb/sqlc/queries/assets.sql Show resolved Hide resolved
tapdb/assets_common.go Show resolved Hide resolved
tapgarden/custodian.go Show resolved Hide resolved
@guggero guggero deleted the balance-bug-import-proofs branch January 8, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants