-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Migrate Equivocation Handling to x/evidence #5299
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5299 +/- ##
==========================================
- Coverage 54.79% 54.79% -0.01%
==========================================
Files 307 311 +4
Lines 18296 18386 +90
==========================================
+ Hits 10026 10075 +49
- Misses 7494 7540 +46
+ Partials 776 771 -5 |
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.
Thanks @alexanderbez for doing this refactor. The distinctions between modules are much clearer now 👏
I left some questions but otherwise LGTM
} | ||
|
||
// reject evidence if the double-sign is too old | ||
if age > k.MaxEvidenceAge(ctx) { |
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.
q: shouldn't every evidence type define its own evidence age instead?
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 don't think so because all evidence must be within the unbonding period. Perhaps certain evidence can have even smaller valid periods (e.g. 1/2 unbonding period), but I don't see any immediate use-cases or evidence types that need 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.
Great work! Small nitpicks
Only important question is why DoubleSignJailTime is treated as a constant rather than as a parameter
Also a heads-up, the Evidence interface got changed in #5321 to accomodate chain misbehaviour. Might require some changes to this PR but nothing substantial. Would just require asserting that the Equivocation struct satified |
Co-Authored-By: Aditya <adityasripal@gmail.com>
Co-Authored-By: Aditya <adityasripal@gmail.com>
@AdityaSripal @fedekunze I've updated and addressed all comments.
Would just require asserting that the Equivocation struct satified How so? What if we merge this PR first? I haven't looked at the PR yet, but I imagine the stricter interface contract is warranted? |
The new The original We can merge this PR first, would require a trivial change to fix things once my PR is merged so i can do that. |
Great! Sounds like a plan. Thanks for the review <3 |
HandleDoubleSign
fromx/slashing
to thex/evidence
modulex/slashing
to thex/evidence
modulex/evidence
client logiccloses: #5296
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry to the
Unreleased
section inCHANGELOG.md
Re-reviewed
Files changed
in the github PR explorerFor Admin Use: