-
Couldn't load subscription status.
- Fork 713
Feat/sip034 #6609
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
Feat/sip034 #6609
Conversation
…reChangeCause so we don't inadvertently represent tenure-changes with extended dimensions that are incompaible with their causes
…n-processing to prevent a tenure-change with a SIP-034 tenure extension from being accepted (thereby invalidating the block)
…ng. WIP: integration test that verifies this
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.
This LGTM once the last integration tests are in place.
…034 tenure extends for the time being
…nt UNIX epoch time from when the block was created in order to surface any potential issues with how the MARF'ed timestamp is generated
… using the UNIX epoch timestamp if it is not given
… a block, so it gets MARF'ed
|
This is ready for review @aaronb-stacks @brice-stacks @jferrant @hstove |
|
@jacinta-stacks @aaronb-stacks @brice-stacks Re-review plz 🙏 |
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.
I was just casually looking through the PR and had a question.
|
Isn't this implementation equivalent to the one produced by the PartialEq
derive macro? Hmmm, maybe the point is to explicitly not implement that
trait? I'm curious, what is missing, incomplete, or a potential footgun if
you were to implement this using the PartialEq trait?
Yes, it's equivalent and intentional. This PR deliberately removed
`PartialEq` from `TenureChangeCause` in order to find and address all cases
where we wanted to consider _any_ tenure extension, but had only compared a
cause variable to `::Extended`. It removes a footgun.
|
|
Whoops, had a failing unit test. Please re-approve @jacinta-stacks @brice-stacks @aaronb-stacks 🙏 |
|
Okay, these integration tests are broken locally for me. Will try and find out why. |
|
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.
This looks great! A few notes that are more on the nit side
8ec9bf9
24e3ace
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (28.69%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## develop #6609 +/- ##
============================================
- Coverage 79.86% 28.69% -51.17%
============================================
Files 573 573
Lines 352853 354061 +1208
============================================
- Hits 281798 101606 -180192
- Misses 71055 252455 +181400
... and 474 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This implements SIP-034. Thanks to @aaronb-stacks for getting the ball rolling on this!
Testing TODO: