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

x/evidence module implementation #5240

Merged
merged 76 commits into from
Nov 6, 2019
Merged

Conversation

alexanderbez
Copy link
Contributor

@alexanderbez alexanderbez commented Oct 24, 2019

The initial x/evidence module implementation per ADR 009 using the latest SDK best practices.

What it does include:

  • Module implementation with core types, methods, functions
  • Module wiring setup
  • Preliminary docs/spec

What it does NOT include (to be accomplished in subsequent PRs):

  • Concrete evidence types
  • Simulation

  • 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 in CHANGELOG.md

  • Re-reviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Oct 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@82a2c5d). Click here to learn what that means.
The diff coverage is 74.35%.

@@            Coverage Diff            @@
##             master    #5240   +/-   ##
=========================================
  Coverage          ?   54.61%           
=========================================
  Files             ?      299           
  Lines             ?    18177           
  Branches          ?        0           
=========================================
  Hits              ?     9928           
  Misses            ?     7464           
  Partials          ?      785

x/evidence/client/cli/query.go Show resolved Hide resolved
x/evidence/client/cli/query.go Outdated Show resolved Hide resolved
x/evidence/client/cli/query.go Outdated Show resolved Hide resolved
x/evidence/client/cli/query.go Outdated Show resolved Hide resolved
x/evidence/client/cli/query.go Show resolved Hide resolved
x/evidence/internal/types/genesis.go Outdated Show resolved Hide resolved
x/evidence/internal/keeper/keeper.go Show resolved Hide resolved
x/evidence/internal/types/test_util.go Show resolved Hide resolved
x/evidence/abci.go Outdated Show resolved Hide resolved
x/evidence/spec/README.md Show resolved Hide resolved
alexanderbez and others added 12 commits November 5, 2019 08:10
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

ACK, very well-documented, thanks.

I am, however, not totally sure how this will be used with IBC - in particular, some of the evidence that the IBC client needs to handle will be of equivocations or other malfeasance committed by validators who are not on this chain. Given that, if the evidence interface methods are assumed to refer to validators on this chain, that may not make sense - we might need to separate out the 'evidence validation' logic and 'evidence punishment' logic. Does that make sense?

@alexanderbez
Copy link
Contributor Author

alexanderbez commented Nov 6, 2019

@cwgoes it is true the Evidence interfaces lends itself to dealing with validators within a source/origin chain, but it is not required to do so as the methods can be no-ops mainly. My intention was to have the Handler be as generic as possible. WRT to IBC, what does the current architecture, i.e. the Handler, not allow you to do that you need in IBC? e.g. a Handler can do validation and/or punishment.

Can you maybe illustrate by example so I have a better understanding of IBC's needs?

@cwgoes
Copy link
Contributor

cwgoes commented Nov 6, 2019

@cwgoes it is true the Evidence interfaces lends itself to dealing with validators within a source/origin chain, but it is not required to do so as the methods can be no-ops mainly. My intention was to have the Handler be as generic as possible. WRT to IBC, what does the current architecture, i.e. the Handler, not allow you to do that you need in IBC? Split it into two? e.g. a Handler can do validation and/or punishment.

Can you maybe illustrate by example so I have a better understanding of IBC's needs?

It's more that the IBC module is designed to call into the evidence module when checkMisbehaviourAndUpdateState is called - see https://github.com/cosmos/ics/tree/master/spec/ics-002-client-semantics#misbehaviour-predicate - I suppose that could be inverted to use the evidence router, but that might be a bit strange since it isn't the pattern in the rest of the IBC module.

If you think that would work with the current structure though it's probably fine.

@alexanderbez alexanderbez merged commit 95ddc24 into master Nov 6, 2019
@alexanderbez alexanderbez deleted the bez/evidence-module-impl branch November 6, 2019 21:08
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants