-
Notifications
You must be signed in to change notification settings - Fork 122
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: fix equivocation e2e-test + fix CLI #2248
Conversation
// the Hermes doesn't support evidence handling for Permissionless ICS yet | ||
// TODO: @Simon refactor once https://github.com/informalsystems/hermes/pull/4182 is merged. | ||
if useRelayer { | ||
// run in detached mode so it will keep running in the background |
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.
If I understood correctly, it seems that if !useRelayer
, we do not run this in the background but check if there's currently evidence. If so, it might make sense to rename startConsumerEvidenceDetector
to something like detectEvidence...
. If that's the case, maybe we should run Hermes in a similar way here and not in detached mode.
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.
Good point! Renamed to detectConsumerEvidenceAction
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 Simon!
LGTM
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.
LGTM
Co-authored-by: insumity <[email protected]>
gas, | ||
) | ||
|
||
cmd = tr.target.ExecCommand( |
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.
why a bash command?
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 find it easier to execute a long command that way.
if err == nil { | ||
evidence := gjson.Get(string(bz), "evidence") | ||
if len(evidence.Array()) > 0 { | ||
infractionHeight = evidence.Array()[0].Get("value.height").Int() |
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.
could it be that the one we're looking for is not the first entry in the list?
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.
We expect only one evidence. I'll update the condition above to reflect this!
log.Fatal("expected at least one evidence in block but found zero") | ||
} | ||
|
||
if equivocation := evidence[0].Get("duplicate_vote_evidence"); equivocation.String() != "" { |
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.
also here, should we iterate over the list until we find a "duplicate_vote_evidence"?
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.
We could but it's simpler to return an error if the evidence isn't of the duplicate vote evidence type since we expect only one evidence.
Co-authored-by: bernd-m <[email protected]>
Make consumer equivocation e2e-test to pass without using Hermes and fix bug in CLI.
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if the change is state-machine breakingCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
the type prefix if the change is state-machine breaking