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

Subtract output values from offset when inscription spent as fee #1971

Merged

Conversation

gmart7t2
Copy link
Contributor

Fixes #1841.

Indexes will need rebuilding.

@veryordinally
Copy link
Collaborator

@raphjaph this looks good to me - should prioritize for review.

@unisky
Copy link

unisky commented Mar 27, 2023

I have found this bug too, it made a big confusion to my current data. hope prioritize for review.

@gmart7t2
Copy link
Contributor Author

I have found this bug too

Your fix is identical to mine too. That's reassuring to see.

@gmart7t2 gmart7t2 force-pushed the fix-tracking-inscriptions-sent-as-fees branch 2 times, most recently from 43be21e to c566985 Compare March 31, 2023 12:28
@unisky
Copy link

unisky commented Apr 3, 2023

hello guys, when to merge this commit and fix history data? I think this bug is very fatal

@gmart7t2 gmart7t2 force-pushed the fix-tracking-inscriptions-sent-as-fees branch 2 times, most recently from ed67c21 to c566985 Compare April 6, 2023 15:56
Copy link
Collaborator

@casey casey left a comment

Choose a reason for hiding this comment

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

I think this change is correct. In addition to a test, can you please describe what the bug is, why this fixes it, and the observable change in behavior? There was a lot of discussion in #1841, but I think it also discusses a different issue, so it's a bit confused. If there's already a different issue describing this, you can also link that issue.

src/index/updater/inscription_updater.rs Show resolved Hide resolved
@gmart7t2
Copy link
Contributor Author

gmart7t2 commented Apr 8, 2023

I think this change is correct. In addition to a test, can you please describe what the bug is, why this fixes it, and the observable change in behavior? There was a lot of discussion in #1841, but I think it also discusses a different issue, so it's a bit confused. If there's already a different issue describing this, you can also link that issue.

The bug is #1841.

https://github.com/casey/ord/issues/1841#issuecomment-1483950070 explains the bug and why this fixes it.

The observable change in behavior is that ord tracks inscriptions correctly when they are spent as change rather than tracking them incorrectly.

I don't understand the confusion here. Sorry if I'm being slow.

@gmart7t2 gmart7t2 closed this Apr 8, 2023
@gmart7t2 gmart7t2 reopened this Apr 8, 2023
@gmart7t2 gmart7t2 requested a review from casey April 9, 2023 00:43
@gmart7t2 gmart7t2 force-pushed the fix-tracking-inscriptions-sent-as-fees branch from 74c2fef to 93e4572 Compare April 12, 2023 14:50
@raphjaph
Copy link
Collaborator

I think the bug is very well explained! I just added the changes you made to the test in a new test so it is very clear what the bug was. This is ready for you to have a look @casey!

@gmart7t2 gmart7t2 force-pushed the fix-tracking-inscriptions-sent-as-fees branch from 7a740c7 to 93e4572 Compare April 14, 2023 14:16
@raphjaph raphjaph force-pushed the fix-tracking-inscriptions-sent-as-fees branch from 90a145c to b97d66e Compare April 14, 2023 18:12
Copy link
Collaborator

@casey casey 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! For bug fixes, existing tests should not be changed if they aren't broken, new tests should be added, which gives confidence that old and correct behavior, covered by the old tests, isn't being changed.

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