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

feat(statemachine)!: Add allow all client wildcard to AllowedClients param #5429

Merged
merged 34 commits into from
Jan 15, 2024

Conversation

sontrinh16
Copy link
Member

@sontrinh16 sontrinh16 commented Dec 17, 2023

Description

All allow all client type wildcard

Overview

closes: #5316


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 and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/).
  • Added relevant godoc comments.
  • Provide a commit message to be used for the changelog entry in the PR description for review.
  • Re-reviewed Files changed in the Github PR explorer.
  • Review Codecov Report in the comment section below once CI passes.

@codecov-commenter
Copy link

codecov-commenter commented Dec 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dd5b172) 81.18% compared to head (93f07ea) 81.19%.
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5429      +/-   ##
==========================================
+ Coverage   81.18%   81.19%   +0.01%     
==========================================
  Files         197      197              
  Lines       15247    15257      +10     
==========================================
+ Hits        12378    12388      +10     
  Misses       2404     2404              
  Partials      465      465              
Files Coverage Δ
modules/core/02-client/types/keys.go 75.00% <ø> (ø)
modules/core/02-client/types/params.go 100.00% <100.00%> (ø)

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM thank you!

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you, @sontrinh16

@damiannolan
Copy link
Member

Is the intension to backport this @crodriguezvega?

Otherwise we'll experience e2e test failures in ibcwasm v7.3 with the e2e diffs in this PR. The gov proposals for adding 08-wasm to allowed clients may need to be wrapped in a feature releases struct to add multi version support

@crodriguezvega
Copy link
Contributor

@damiannolan I think with these changes it is working. I took the code from this other PR, so I will add @expertdicer as co-author when I merge it.

@crodriguezvega crodriguezvega changed the title feat: Add allow all client wildcard to AllowedClients param feat(statemachine)!: Add allow all client wildcard to AllowedClients param Dec 20, 2023
@phamminh0811
Copy link
Contributor

phamminh0811 commented Dec 21, 2023

Hi @crodriguezvega, I think that I'm the main author of this PR, not @expertdicer.

@expertdicer
Copy link
Contributor

Our avatars have some differents. 😄

@crodriguezvega
Copy link
Contributor

Our avatars have some differents. 😄

oh, sorry, yes. The icons looked the same in small size. 😅

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Thank you! 🚀


// AllowAllClientsWildcardFeatureReleases represents the releases the allow all clients wildcard was released in.
var AllowAllClientsWildcardFeatureReleases = semverutil.FeatureReleases{
MajorVersion: "v9",
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we add this feature in v8.1?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could!

Copy link
Member

Choose a reason for hiding this comment

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

I left v9 as the major version and added v8.1 in the minor versions.

Also added the backport label

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me update the docs before merging, because we probably have a few places where we should now mention that you don't need to update the allowed clients if you add a new client.

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

ACK docs changes, thanks @crodriguezvega

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.

sweet! 🍬

@damiannolan damiannolan merged commit d5949b1 into cosmos:main Jan 15, 2024
70 checks passed
mergify bot pushed a commit that referenced this pull request Jan 15, 2024
…param (#5429)

* add wildcard allow all client

* minor

* fix lint

* minor and doc update

* review comments

* if allow all clients is present, no other client types should be in the allow list

* clean up code to allow wasm client type

* remove unused variable

* Fix build failures, tweak err message.

* modify allow clients list in genesis with feature releases

* chore: adding v8.1 to minor versions in e2e feat releases struct

* update docs

* chore: doc lint fixes

---------

Co-authored-by: GnaD <89174180+GNaD13@users.noreply.github.com>
Co-authored-by: Du Nguyen <61083705+lichdu29@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Co-authored-by: Đỗ Việt Hoàng <hoangdv2429@gmail.com>
Co-authored-by: Charly <charly@interchain.io>
(cherry picked from commit d5949b1)

# Conflicts:
#	docs/docs/03-light-clients/04-wasm/03-integration.md
#	e2e/tests/wasm/grandpa_test.go
#	e2e/testsuite/testconfig.go
#	e2e/testvalues/values.go
#	modules/core/02-client/types/params.go
#	modules/core/02-client/types/params_test.go
#	modules/light-clients/08-wasm/keeper/keeper_test.go
#	modules/light-clients/08-wasm/testing/wasm_endpoint.go
#	modules/light-clients/08-wasm/types/types_test.go
crodriguezvega added a commit that referenced this pull request Jan 15, 2024
…param (backport #5429) (#5609)

* feat(statemachine)!: Add allow all client wildcard to AllowedClients param (#5429)

* add wildcard allow all client

* minor

* fix lint

* minor and doc update

* review comments

* if allow all clients is present, no other client types should be in the allow list

* clean up code to allow wasm client type

* remove unused variable

* Fix build failures, tweak err message.

* modify allow clients list in genesis with feature releases

* chore: adding v8.1 to minor versions in e2e feat releases struct

* update docs

* chore: doc lint fixes

---------

Co-authored-by: GnaD <89174180+GNaD13@users.noreply.github.com>
Co-authored-by: Du Nguyen <61083705+lichdu29@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Co-authored-by: DimitrisJim <d.f.hilliard@gmail.com>
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Co-authored-by: Đỗ Việt Hoàng <hoangdv2429@gmail.com>
Co-authored-by: Charly <charly@interchain.io>
(cherry picked from commit d5949b1)

# Conflicts:
#	docs/docs/03-light-clients/04-wasm/03-integration.md
#	e2e/tests/wasm/grandpa_test.go
#	e2e/testsuite/testconfig.go
#	e2e/testvalues/values.go
#	modules/core/02-client/types/params.go
#	modules/core/02-client/types/params_test.go
#	modules/light-clients/08-wasm/keeper/keeper_test.go
#	modules/light-clients/08-wasm/testing/wasm_endpoint.go
#	modules/light-clients/08-wasm/types/types_test.go

* chore: rm -rf e2e

* chore: rm -rf modules/light-clients/08-wasm

* chore: fix conflicts in 02-client/types/params and tests

* fix: validateClients duplicates

* chore: rm -rf docs

* chore: remove markdown link checker workflow from release branch

* add changelog

---------

Co-authored-by: son trinh <trinhleson2000@gmail.com>
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
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.

Add allow all wildcard for allowed clients