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

Introduce new AccessType to allow a set of addresses #974

Merged
merged 5 commits into from
Sep 8, 2022

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Sep 2, 2022

Resolves #945

Introduces a new Access type to allow a set of addresses to accept.

This PR includes a small but breaking change to the CLI. The separator char between codeID and permission is : now:
Before:

wasmd tx gov submit-proposal update-instantiate-config 1,nobody 2,everybody 3,wasm1l2rsakp388kuv9k8qzq6lrm9taddae7fpx59wm

Now:

wasmd tx gov submit-proposal update-instantiate-config 1:nobody 2:everybody 3:wasm1l2rsakp388kuv9k8qzq6lrm9taddae7fpx59wm,wasm1vx8knpllrj7n963p9ttd80w47kpacrhuts497x`
  • keep or migrate AccessConfig.Address into AccessConfig.Addresses see proto? - keep and remove in a future version

@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #974 (20cd4af) into main (f430ca3) will increase coverage by 1.18%.
The diff coverage is 81.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #974      +/-   ##
==========================================
+ Coverage   59.29%   60.48%   +1.18%     
==========================================
  Files          51       51              
  Lines        6238     6311      +73     
==========================================
+ Hits         3699     3817     +118     
+ Misses       2272     2224      -48     
- Partials      267      270       +3     
Impacted Files Coverage Δ
x/wasm/keeper/proposal_handler.go 68.00% <ø> (ø)
x/wasm/client/cli/tx.go 42.62% <73.80%> (+17.62%) ⬆️
x/wasm/client/cli/gov_tx.go 6.52% <83.33%> (+6.52%) ⬆️
x/wasm/types/params.go 71.42% <86.11%> (+11.21%) ⬆️
x/wasm/types/types.go 60.20% <89.47%> (+2.42%) ⬆️
x/wasm/client/cli/genesis_msg.go 72.06% <100.00%> (+0.09%) ⬆️
x/wasm/keeper/keeper.go 88.57% <0.00%> (+0.33%) ⬆️

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good.
Added some minor style comments.
Seems one whole-system test is missing (that actually executes keeper logic with AnyOf), but otherwise good to merge from my side.

@@ -19,12 +19,16 @@ enum AccessType {
// AccessTypeNobody forbidden
ACCESS_TYPE_NOBODY = 1
[ (gogoproto.enumvalue_customname) = "AccessTypeNobody" ];
// AccessTypeOnlyAddress restricted to an address
// AccessTypeOnlyAddress restricted to a single address
// Deprecated: use AccessTypeAnyOfAddresses instead
Copy link
Member

Choose a reason for hiding this comment

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

Why deprecate?
I think it is fine to use the simpler version for the cases where it works

proto/cosmwasm/wasm/v1/types.proto Show resolved Hide resolved
proto/cosmwasm/wasm/v1/types.proto Show resolved Hide resolved
x/wasm/types/params.go Show resolved Hide resolved
x/wasm/types/params_test.go Show resolved Hide resolved
x/wasm/types/types.go Show resolved Hide resolved
x/wasm/types/types.go Show resolved Hide resolved
@ethanfrey ethanfrey added this to the v0.29.0 milestone Sep 6, 2022
@alpe alpe mentioned this pull request Sep 7, 2022
3 tasks
@alpe alpe marked this pull request as ready for review September 7, 2022 14:23
Copy link
Contributor

@jhernandezb jhernandezb left a comment

Choose a reason for hiding this comment

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

Looks good to me

Short: "Submit an update instantiate config proposal.",
Args: cobra.MinimumNArgs(1),
Long: strings.TrimSpace(
fmt.Sprintf(`Submit an update instantiate config proposal for multiple code ids.

Example:
$ %s tx gov submit-proposal update-instantiate-config 1,nobody 2,everybody 3,%s1l2rsakp388kuv9k8qzq6lrm9taddae7fpx59wm
`, version.AppName, bech32Prefix)),
$ %s tx gov submit-proposal update-instantiate-config 1:nobody 2:everybody 3:%s1l2rsakp388kuv9k8qzq6lrm9taddae7fpx59wm,%s1vx8knpllrj7n963p9ttd80w47kpacrhuts497x
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! I think this should work fine.

},
}
for name, spec := range specs {
t.Run(name, func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice set of tests!

@alpe
Copy link
Contributor Author

alpe commented Sep 8, 2022

Thanks for the feedback!

@alpe alpe merged commit 1c4a5bd into main Sep 8, 2022
@alpe alpe deleted the 945_access_config_anyof branch May 10, 2023 08:54
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.

Allow AccessConfig to use a list of addresses instead of just a single address
3 participants