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

02-client refactor: creating ValidateClientMessage fn for Header and Misbehaviour on 07-tendermint #1115

Conversation

seantking
Copy link
Contributor

@seantking seantking commented Mar 13, 2022

Description

This PR is merging into #1107 as it depends on the use of the new ClientMessage type. Once #1107 is merged I will point to 02-client feature branch.

  • Renaming checkValidity to ValidateClientMessage and exposing on ClientState
  • ValidateClientMessage now validates a Header or a Misbehaviour
  • Using ValidateClientMessage in both CheckMisbehaviourAndUpdateState & CheckHeaderAndUpdateState

partially closes: #879


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

colin-axner and others added 30 commits February 10, 2022 11:07
… application in middleware stack (#892)

## Description

Currently the `AppModule` assumes a single scoped keeper. This doesn't allow the mock module to be used as a base application for different middleware stack (ica stack, fee stack, etc)

I broke the API because I think it is cleaner. If we want this to be non API breaking, I can try to readjust

ref: #891 

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
* feat: adding Pack/Unpack acknowledgement helper fns

* chore: changelog

* fix: docs

* Update modules/core/04-channel/types/codec.go

Co-authored-by: colin axnér <[email protected]>

Co-authored-by: colin axnér <[email protected]>
## Description



ref: #891

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
…ts (#921)

## Description

This contains two fixes:
- the capability being claimed by the scoped keeper was incorrect (mock.ModuleName -> port ID)
- the mock module wasn't accounting for non empty genesis state in capabilities (after genesis export, capability will create the bound ports so rebinding doesn't need to happen)

closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
…#920)

## Description



closes: #XXXX

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
* chore(ica): add trail of bits audit report

* relocate the audit report for ICA

Co-authored-by: Carlos Rodriguez <[email protected]>
* testing: adding multiple sender accounts for testing puproses

* fix genesis setup (#936)

* Update testing/chain.go

Co-authored-by: colin axnér <[email protected]>

* refactor: code hygiene

* Update testing/chain.go

Co-authored-by: Aditya <[email protected]>

* fix: setting totalySupply to empty

* nit: CamelCase not UPPERCASE

Co-authored-by: Aditya <[email protected]>
Co-authored-by: colin axnér <[email protected]>
* testing: adding multiple sender accounts for testing puproses

* fix genesis setup (#936)

* Update testing/chain.go

Co-authored-by: colin axnér <[email protected]>

* refactor: code hygiene

* Update testing/chain.go

Co-authored-by: Aditya <[email protected]>

* multi validator commit taken from @Saione

* add function to pass custom valset

* add changelog

Co-authored-by: Sean King <[email protected]>
Co-authored-by: Sean King <[email protected]>
Co-authored-by: colin axnér <[email protected]>
## Description



closes: #850 

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
…973)

This reverts commit 843b459.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
* chore: update migration docs

* Update docs/migrations/v2-to-v3.md

Co-authored-by: Damian Nolan <[email protected]>

Co-authored-by: Damian Nolan <[email protected]>
* bug: support base denoms with slashes

* add changelog entry

Co-authored-by: Carlos Rodriguez <[email protected]>
* upgrade ics23 to v0.7-rc

* add changelog entry

* update ics23 to final 0.7

Co-authored-by: Carlos Rodriguez <[email protected]>
…oposal (#1037)

## Description



closes: #1034 

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [ ] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
## Description



This add some missing arguments to `ibckeeper.NewKeeper` and `ibctransferkeeper.NewKeeper` in integration docs

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

- [x] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules/structure.md).
- [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing)
- [x] Updated relevant documentation (`docs/`) or specification (`x/<module>/spec/`)
- [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code).
- [ ] Added a relevant changelog entry to the `Unreleased` section in `CHANGELOG.md`
- [ ] Re-reviewed `Files changed` in the Github PR explorer
- [ ] Review `Codecov Report` in the comment section below once CI passes
* small fixes for v2 to v3 migration

* review comment

* Update v2-to-v3.md

* add store upgrade documentation

Co-authored-by: Carlos Rodriguez <[email protected]>
Bumps [actions/checkout](https://github.com/actions/checkout) from 2.4.0 to 3.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/actions/checkout/releases">actions/checkout's releases</a>.</em></p>
<blockquote>
<h2>v3.0.0</h2>
<ul>
<li>Update default runtime to node16</li>
</ul>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a href="https://github.com/actions/checkout/blob/main/CHANGELOG.md">actions/checkout's changelog</a>.</em></p>
<blockquote>
<h1>Changelog</h1>
<h2>v2.3.1</h2>
<ul>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/284">Fix default branch resolution for .wiki and when using SSH</a></li>
</ul>
<h2>v2.3.0</h2>
<ul>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/278">Fallback to the default branch</a></li>
</ul>
<h2>v2.2.0</h2>
<ul>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/258">Fetch all history for all tags and branches when fetch-depth=0</a></li>
</ul>
<h2>v2.1.1</h2>
<ul>
<li>Changes to support GHES (<a href="https://github-redirect.dependabot.com/actions/checkout/pull/236">here</a> and <a href="https://github-redirect.dependabot.com/actions/checkout/pull/248">here</a>)</li>
</ul>
<h2>v2.1.0</h2>
<ul>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/191">Group output</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/199">Changes to support GHES alpha release</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/184">Persist core.sshCommand for submodules</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/163">Add support ssh</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/179">Convert submodule SSH URL to HTTPS, when not using SSH</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/157">Add submodule support</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/144">Follow proxy settings</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/141">Fix ref for pr closed event when a pr is merged</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/128">Fix issue checking detached when git less than 2.22</a></li>
</ul>
<h2>v2.0.0</h2>
<ul>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/108">Do not pass cred on command line</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/107">Add input persist-credentials</a></li>
<li><a href="https://github-redirect.dependabot.com/actions/checkout/pull/104">Fallback to REST API to download repo</a></li>
</ul>
<h2>v2 (beta)</h2>
<ul>
<li>Improved fetch performance
<ul>
<li>The default behavior now fetches only the SHA being checked-out</li>
</ul>
</li>
<li>Script authenticated git commands
<ul>
<li>Persists <code>with.token</code> in the local git config</li>
<li>Enables your scripts to run authenticated git commands</li>
<li>Post-job cleanup removes the token</li>
<li>Coming soon: Opt out by setting <code>with.persist-credentials</code> to <code>false</code></li>
</ul>
</li>
<li>Creates a local branch
<ul>
<li>No longer detached HEAD when checking out a branch</li>
<li>A local branch is created with the corresponding upstream branch set</li>
</ul>
</li>
<li>Improved layout</li>
</ul>

</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/actions/checkout/commit/a12a3943b4bdde767164f792f33f40b04645d846"><code>a12a394</code></a> update readme for v3 (<a href="https://github-redirect.dependabot.com/actions/checkout/issues/708">#708</a>)</li>
<li><a href="https://github.com/actions/checkout/commit/8f9e05e482293f862823fcca12d9eddfb3723131"><code>8f9e05e</code></a> Update to node 16 (<a href="https://github-redirect.dependabot.com/actions/checkout/issues/689">#689</a>)</li>
<li><a href="https://github.com/actions/checkout/commit/230611dbd0eb52da1e1f4f7bc8bb0c3a339fc8b7"><code>230611d</code></a> Change secret name for PAT to not start with GITHUB_ (<a href="https://github-redirect.dependabot.com/actions/checkout/issues/623">#623</a>)</li>
<li>See full diff in <a href="https://github.com/actions/checkout/compare/v2.4.0...v3">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=actions/checkout&package-manager=github_actions&previous-version=2.4.0&new-version=3)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)


</details>
* add counterpartyChannelID param to IBCModule OnChanOpenAck()

* change testing mock

* change ica IBCModules ChannelOpenAck

* change transfer IBCModules ChannelOpenAck

* change core keeper ChannelOpenAck()

* CHANGELOG.md

* update v2-to-v3 migration doc

* Update docs/migrations/v2-to-v3.md

Co-authored-by: Carlos Rodriguez <[email protected]>

Co-authored-by: Carlos Rodriguez <[email protected]>
fedekunze and others added 8 commits March 9, 2022 19:29
* replace channel keeper with IBC keeper in AnteDecorator and pass message to rpc handler

* fix error checking condition

* fix for proper way of getting go context

* refactor tests for ante handler

* review comment

* review comments and some fixes

* review comments

* execute message for update client as well

* add migration

Co-authored-by: Carlos Rodriguez <[email protected]>
* ibctesting: custom voting power reduction for testing

* changelog

* fix

* make T public

* fix

* revert changes

* fix test
@seantking seantking force-pushed the sean/issue#879-rename-check-validity-verify-header branch from f750072 to f45fd50 Compare March 13, 2022 11:58
@codecov-commenter
Copy link

codecov-commenter commented Mar 13, 2022

Codecov Report

Merging #1115 (b76e338) into sean/issue#875-merge-misbehvior-header-interfaces (c345354) will increase coverage by 0.02%.
The diff coverage is 90.00%.

Impacted file tree graph

@@                                  Coverage Diff                                  @@
##           sean/issue#875-merge-misbehvior-header-interfaces    #1115      +/-   ##
=====================================================================================
+ Coverage                                              79.69%   79.72%   +0.02%     
=====================================================================================
  Files                                                    151      151              
  Lines                                                  10884    10899      +15     
=====================================================================================
+ Hits                                                    8674     8689      +15     
  Misses                                                  1786     1786              
  Partials                                                 424      424              
Impacted Files Coverage Δ
...odules/light-clients/07-tendermint/types/update.go 84.73% <89.77%> (+2.86%) ⬆️
...clients/07-tendermint/types/misbehaviour_handle.go 80.32% <100.00%> (-3.89%) ⬇️

clientState *ClientState, consState *ConsensusState,
header *Header, currentTimestamp time.Time,
// ValidateClientMessage checks if the clientMessage is of type Header or Misbehaviour and validates the message
func (cs *ClientState) ValidateClientMessage(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colin-axner @damiannolan Open to ideas regarding the fn parameters here. It was necessary to pass in ctx, cdc, clientStore for the Misbehaviour type but perhaps there is a way around this.

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to settle on the APIs of the new client state methods. It's likely that 06-solomachine will not make use of all the args that 07-tendermint does, but it will still require them to be provided to fulfil the expected interface.

Copy link
Contributor

@colin-axner colin-axner Mar 14, 2022

Choose a reason for hiding this comment

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

It should be the same as the existing interface function (except the return value should just be an error

@seantking seantking changed the title refactor: creating validateClientMessage fn for Header and Misbehaviour 02-client refactor: refactor: creating ValidateClientMessage fn for Header and Misbehaviour on 07-tendermint Mar 13, 2022
@seantking seantking changed the title 02-client refactor: refactor: creating ValidateClientMessage fn for Header and Misbehaviour on 07-tendermint 02-client refactor: creating ValidateClientMessage fn for Header and Misbehaviour on 07-tendermint Mar 13, 2022
&cs, tmConsensusState2, tmMisbehaviour.Header2, ctx.BlockTime(),
); err != nil {
return nil, sdkerrors.Wrap(err, "verifying Header2 in Misbehaviour failed")
if err := cs.ValidateClientMessage(ctx, clientStore, cdc, nil, misbehaviour, ctx.BlockTime()); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colin-axner Decided to go with ValidateClientMessage instead of VerifyClientMessage as per a suggestion from @damiannolan . wdyt?

Copy link
Member

@damiannolan damiannolan Mar 14, 2022

Choose a reason for hiding this comment

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

Yeah I'm still unsure of the naming. I suggested using ValidateClientMessage instead of VerifyClientMessage to have some separation between this and other VerifyXyz methods on the ClientState interface which do proof verification (the ones listed under "State verification functions")

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be VerifyClientMessage. This function is doing proof verification. Validate usage in SDK modules implies basic validation

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes okay, so a general rule of thumb should be "validate" is used in terms of stateless checks

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes exactly

@seantking seantking mentioned this pull request Mar 13, 2022
3 tasks
return err
}
switch header.(type) {
case *Header:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we should make private verifyHeader and verifyMisbehaviour functions

Base automatically changed from sean/issue#875-merge-misbehvior-header-interfaces to 02-client-refactor March 15, 2022 10:37
@seantking
Copy link
Contributor Author

Closed in favor of #1119 due to github UI being annoying 🤷

@seantking seantking closed this Mar 15, 2022
@seantking seantking deleted the sean/issue#879-rename-check-validity-verify-header branch August 3, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants