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.

Sign up for GitHub

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

Jump to bottom

imp(02-client,03-connection)!: remove ValidateSelfClient #6853

Merged
merged 4 commits into from
Jul 19, 2024
Merged

imp(02-client,03-connection)!: remove ValidateSelfClient #6853

merged 4 commits into from
Jul 19, 2024

Conversation

srdtrk
Copy link
Member

@srdtrk srdtrk commented Jul 17, 2024

Description

This addresses the spec changes proposed in cosmos/ibc#1121

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 the 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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@damiannolan
Copy link
Contributor

Is the idea to remove GetSelfConsensusState in a followup PR? The entire ConsensusHost interface can be removed.

@srdtrk
Copy link
Member Author

srdtrk commented Jul 17, 2024

Yes @damiannolan. I didn't wanna have too much diff in a single PR. Plus, I didn't wanna deal with any potential side-effects in this PR.

@srdtrk
Copy link
Member Author

srdtrk commented Jul 17, 2024

Would everyone prefer me to do this here?

@damiannolan
Copy link
Contributor

Would everyone prefer me to do this here?

Fine with doing it in two steps. Hopefully the next one doesn't cause any hassle!

@damiannolan damiannolan mentioned this pull request Jul 17, 2024
Closed
3 tasks
crodriguezvega
crodriguezvega reviewed Jul 17, 2024
View reviewed changes
Copy link
Contributor

@crodriguezvega crodriguezvega left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, @srdtrk. So just to confirm: we want to release these changes already in v9, right?

We should add a changelog and also update the migration docs with the removal of ValidateSelfClient and GetSelfConsensusState functions in 02-client keeper and the removal of ConsensusHost interface (since that was introduced in v8.3).

@srdtrk
Copy link
Member Author

srdtrk commented Jul 17, 2024

I'd say we want them in v9. Maybe you can ask Aditya if they found an issue with this. We can add the docs on the subsequent PR where we remove the entire ConsensusHost @crodriguezvega ?

colin-axner
colin-axner approved these changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@colin-axner colin-axner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK changes, but would like to hold off merging until we get the analysis from @AdityaSripal. Maybe we can have a feat branch in the meantime?

@crodriguezvega
Copy link
Contributor

I will restrain from adding the backport label until we are given the green light to merge!

DimitrisJim
DimitrisJim approved these changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

🧹 🔴

@srdtrk srdtrk changed the base branch from main to feat/rm-cs-validation July 19, 2024 06:08
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed for 'ibc-go'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@srdtrk srdtrk merged commit dce76d2 into feat/rm-cs-validation Jul 19, 2024
65 checks passed
@srdtrk srdtrk deleted the serdar/xxx-rm-validate-self branch July 19, 2024 07:22
@srdtrk
Copy link
Member Author

srdtrk commented Jul 19, 2024

Merged to a feature branch

github-merge-queue bot pushed a commit that referenced this pull request Aug 6, 2024
@srdtrk @crodriguezvega @colin-axner
imp!: removed 'ConsensusHost' interface (#6937)
2028e9a 
* imp(02-client,03-connection)!: remove `ValidateSelfClient` (#6853)

* imp: removed ValidateSelfClient

* docs: updated a godoc

* imp: deleted consensus host from core

* imp(08-wasm): removed consensus host

* imp: fix linter

* imp: fixed linter

* imp: fixed simapp

* imp: updated docs

* imp: removed more code

* revert

* imp: removed unneeded proto fields

* imp: lint

* lint

* imp: auto generated code

* imp: removed conflicts

* imp: removed more code

* fix: tests

* feat: all tests passing

* fix: added the reserved proto fields back with deprecation notice

* style: linted

* imp: regenerated proto

* imp: review item

* revert: conn name change

* docs: added changelog

* add godoc string of QueryConnectionHandshakeProof

* add migration docs for ibc-go

* Update CHANGELOG.md

* update changelog

* imp(proto): added deprecation notice to field

* imp: ran 'make proto-all'

* imp: removed unused keeper

* Update CHANGELOG.md

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

* Update docs/docs/05-migrations/13-v8-to-v9.md

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

* Update docs/docs/05-migrations/13-v8-to-v9.md

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

---------

Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: colin axnér <[email protected]>
mergify bot pushed a commit that referenced this pull request Aug 6, 2024
@srdtrk @mergify
imp!: removed 'ConsensusHost' interface (#6937)
dd18f5f 
* imp(02-client,03-connection)!: remove `ValidateSelfClient` (#6853)

* imp: removed ValidateSelfClient

* docs: updated a godoc

* imp: deleted consensus host from core

* imp(08-wasm): removed consensus host

* imp: fix linter

* imp: fixed linter

* imp: fixed simapp

* imp: updated docs

* imp: removed more code

* revert

* imp: removed unneeded proto fields

* imp: lint

* lint

* imp: auto generated code

* imp: removed conflicts

* imp: removed more code

* fix: tests

* feat: all tests passing

* fix: added the reserved proto fields back with deprecation notice

* style: linted

* imp: regenerated proto

* imp: review item

* revert: conn name change

* docs: added changelog

* add godoc string of QueryConnectionHandshakeProof

* add migration docs for ibc-go

* Update CHANGELOG.md

* update changelog

* imp(proto): added deprecation notice to field

* imp: ran 'make proto-all'

* imp: removed unused keeper

* Update CHANGELOG.md

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

* Update docs/docs/05-migrations/13-v8-to-v9.md

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

* Update docs/docs/05-migrations/13-v8-to-v9.md

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

---------

Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: colin axnér <[email protected]>
(cherry picked from commit 2028e9a)

# Conflicts:
#	CHANGELOG.md
#	docs/docs/01-ibc/02-integration.md
#	docs/docs/05-migrations/13-v8-to-v9.md
#	modules/core/02-client/keeper/keeper.go
#	modules/core/keeper/keeper.go
#	modules/light-clients/08-wasm/CHANGELOG.md
DimitrisJim pushed a commit that referenced this pull request Aug 7, 2024
@mergify @srdtrk @colin-axner
imp!: removed 'ConsensusHost' interface (backport #6937) (#7057)
ff78dea 
* imp!: removed 'ConsensusHost' interface (#6937)

* imp(02-client,03-connection)!: remove `ValidateSelfClient` (#6853)

* imp: removed ValidateSelfClient

* docs: updated a godoc

* imp: deleted consensus host from core

* imp(08-wasm): removed consensus host

* imp: fix linter

* imp: fixed linter

* imp: fixed simapp

* imp: updated docs

* imp: removed more code

* revert

* imp: removed unneeded proto fields

* imp: lint

* lint

* imp: auto generated code

* imp: removed conflicts

* imp: removed more code

* fix: tests

* feat: all tests passing

* fix: added the reserved proto fields back with deprecation notice

* style: linted

* imp: regenerated proto

* imp: review item

* revert: conn name change

* docs: added changelog

* add godoc string of QueryConnectionHandshakeProof

* add migration docs for ibc-go

* Update CHANGELOG.md

* update changelog

* imp(proto): added deprecation notice to field

* imp: ran 'make proto-all'

* imp: removed unused keeper

* Update CHANGELOG.md

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

* Update docs/docs/05-migrations/13-v8-to-v9.md

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

* Update docs/docs/05-migrations/13-v8-to-v9.md

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

---------

Co-authored-by: Carlos Rodriguez <[email protected]>
Co-authored-by: colin axnér <[email protected]>
(cherry picked from commit 2028e9a)

# Conflicts:
#	CHANGELOG.md
#	docs/docs/01-ibc/02-integration.md
#	docs/docs/05-migrations/13-v8-to-v9.md
#	modules/core/02-client/keeper/keeper.go
#	modules/core/keeper/keeper.go
#	modules/light-clients/08-wasm/CHANGELOG.md

* fix merge conflicts, changelog, delete docs, keeper fns

---------

Co-authored-by: srdtrk <[email protected]>
Co-authored-by: Colin Axnér <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crodriguezvega crodriguezvega crodriguezvega left review comments

@gjermundgaraba gjermundgaraba gjermundgaraba approved these changes

@damiannolan damiannolan damiannolan approved these changes

@DimitrisJim DimitrisJim DimitrisJim approved these changes

@colin-axner colin-axner colin-axner approved these changes

@AdityaSripal AdityaSripal Awaiting requested review from AdityaSripal

@chatton chatton Awaiting requested review from chatton

@bznein bznein Awaiting requested review from bznein

Assignees
No one assigned
Labels
02-client 03-connection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@srdtrk @damiannolan @crodriguezvega @gjermundgaraba @DimitrisJim @colin-axner