-
Notifications
You must be signed in to change notification settings - Fork 679
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: burn-block-height
inside of at-block
#5524
base: develop
Are you sure you want to change the base?
fix: burn-block-height
inside of at-block
#5524
Conversation
burn-block-height
inside of at-block
burn-block-height
inside of at-block
2e861a2
to
ffc5819
Compare
NOTE: We need to include this in a release before 3.1 activates, or else it will require an additional hard-fork. The @kantai do you have any thoughts for a better way to handle this? |
Right — I think the best thing to do might be to add an instance variable to the underlying store like “current burn block” — this would be set to the burn view when block processing begins. The at-block call would then handle looking up the burn view at the at-block target, and set the current burn block at that point, resetting it on return. This would be a bit more complicated, but it would separate the logic for setting burn block height between at block and normal operation. |
let epoch = self.get_clarity_epoch_version()?; | ||
match epoch { | ||
// Special case to preserve possibly incorrect behavior (inside at-block) in Epoch 3.0 | ||
StacksEpochId::Epoch30 => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StacksEpochId::Epoch30 => { | |
StacksEpochId::Epoch30 | StacksEpochId::Epoch31 => { |
(Seems obvious, but let's not forget about it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why we avoid using the _
case in stacks-core... we can't forget about it. May want to practice the same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when I created this PR I was hoping to get it into Epoch 3.1, but that opportunity has passed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why we avoid using the _ case in stacks-core
Oh right, there should be a clippy rule about that but I can't find one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's allowed by default and can't be set in clippy.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok. There were some arguments made about not applying that rule any way, but in most cases, we should manually follow that rule.
Description
Fix
burn-block-height
keyword inside ofat-block
context. See hirosystems/clarinet#1615 for detailsApplicable issues
Additional info (benefits, drawbacks, caveats)
I'm a little unsure of this this part
I had to do this so unit tests would pass, but we get the expected behavior in Clarinet:
Checklist
docs/rpc/openapi.yaml
andrpc-endpoints.md
for v2 endpoints,event-dispatcher.md
for new events)clarity-benchmarking
repobitcoin-tests.yml