-
Notifications
You must be signed in to change notification settings - Fork 411
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
Add store code authz #1591
Add store code authz #1591
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1591 +/- ##
==========================================
- Coverage 56.59% 56.58% -0.02%
==========================================
Files 64 64
Lines 8755 8886 +131
==========================================
+ Hits 4955 5028 +73
- Misses 3414 3468 +54
- Partials 386 390 +4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start! I have added some quick feedback
x/wasm/types/authz.go
Outdated
} | ||
|
||
// NewAuthz factory method to create an Authorization with updated grants | ||
func (a StoreCodeAuthorization) NewAuthz(g []CodeGrant) authztypes.Authorization { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as NewStoreCodeAuthorization
. Do you need a second constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. Good progress!
I have added some nits but also comments to spec out some edge cases.
x/wasm/types/authz_test.go
Outdated
}{ | ||
"all good": { | ||
setup: func(t *testing.T) validatable { | ||
t.Helper() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this t.Helper()
not very helpful in table tests therefore I removed the linter. Let's keep setup
functions to the minimum
expErr: sdkerrors.ErrInvalidRequest, | ||
}, | ||
} | ||
for name, spec := range specs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress!
x/wasm/ioutils/ioutil.go
Outdated
) | ||
|
||
var errLimit = errorsmod.Register("wasm", 29, "exceeds limit") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not register a new error type outside of types/errors . You can just make this a regular stdlib error. In the method call you can check for the concrete type if needed and use a registered type. Please make sure that this error type is public
x/wasm/types/authz.go
Outdated
// TODO: consume gas | ||
wasmCode, err := ioutils.Uncompress(code, int64(MaxWasmSize)) | ||
if err != nil { | ||
return authztypes.AcceptResponse{}, sdkerrors.ErrInvalidRequest.Wrap(errorsmod.Wrap(err, "uncompress wasm archive").Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line would work with a Go stdlib error without a change. The error message is wrapped into a ErrInvalidRequest
type. The registered errLimit
namespace/code is ignored anyway.
x/wasm/types/authz.go
Outdated
return errorsmod.Wrapf(err, "position %d", i) | ||
} | ||
for j := i + 1; j < len(a.Grants); j++ { | ||
if strings.EqualFold(string(a.Grants[i].CodeHash), string(a.Grants[i].CodeHash)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always true. One of the grants should be found by j
.
Alternatively, you can store the codehashes in a map and compare the len to be equal len(grants)
x/wasm/types/authz.go
Outdated
const gasDeserializationCostPerByte = uint64(1) | ||
const ( | ||
gasDeserializationCostPerByte = uint64(1) | ||
codehashWildcard = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make the wildcard public so that it can be used from other packages if needed
9c205fe
to
ade4fd9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good progress. 🏄
default: | ||
uniqueGrants := make(map[string]struct{}) | ||
for i, grant := range a.Grants { | ||
if strings.EqualFold(string(grant.CodeHash), CodehashWildcard) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice use of switch len() here so that you always have >1 elements here for the wilcard check 👍
if err := grant.ValidateBasic(); err != nil { | ||
return errorsmod.Wrapf(err, "position %d", i) | ||
} | ||
uniqueGrants[strings.ToLower(string(grant.CodeHash))] = struct{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good normalization
x/wasm/keeper/keeper.go
Outdated
@@ -145,6 +145,11 @@ func (k Keeper) GetAuthority() string { | |||
return k.authority | |||
} | |||
|
|||
// GetGasRegister returns the x/wasm module's gas register. | |||
func (k Keeper) GetGasRegister() GasRegister { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 makes sense
x/wasm/types/authz.go
Outdated
permission := *storeMsg.InstantiatePermission | ||
|
||
if ioutils.IsGzip(code) { | ||
gasRegister, ok := GasRegister(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! 🥇
import "google/protobuf/any.proto"; | ||
import "amino/amino.proto"; | ||
|
||
option go_package = "github.com/CosmWasm/wasmd/x/wasm/types"; | ||
option (gogoproto.goproto_getters_all) = false; | ||
|
||
// StoreCodeAuthorization defines authorization for wasm code upload. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Since: wasmd 0.42
* main: Add store code authz (#1591) Handle query for non ibc contracts Test channels query Start rework channel query Restrict pagination on all state query
message CodeGrant { | ||
// CodeHash is the unique identifier created by wasmvm | ||
// Wildcard "*" is used to specify any kind of grant. | ||
bytes code_hash = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why this is not a string? All checksums are lower case hex string in the CosmWasm stack when they are visible for users. This thing needs to be displayed as part of transaction signing.
What kind of bytes are expected here? 32 raw bytes hash output or the hex string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it was kept as bytes to keep consistency with other code_hash/checksum attributes in the proto files such as:
message CodeInfo {
// CodeHash is the unique identifier created by wasmvm
bytes code_hash = 1;
and
message MsgStoreCodeResponse {
// CodeID is the reference to the stored WASM code
uint64 code_id = 1 [ (gogoproto.customname) = "CodeID" ];
// Checksum is the sha256 hash of the stored code
bytes checksum = 2;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but please note that all bytes fields are problematic for transaction signing. "*" will be shown as code_hash : Kg==
to the user when verifying a transaction. An actual hash is shown as an unreadable base64 string.
Not much we can do here now since this is released I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I will be more careful in the future.
Maybe in future releases we can fix all these fields and have migrations for them. I don't think it will be too difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. We have similar issues in many places in the stack. We'll figure it out eventually.
Resolves #1584