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

Extend ContractInfo for custom data #492

Merged
merged 2 commits into from
Apr 16, 2021
Merged

Extend ContractInfo for custom data #492

merged 2 commits into from
Apr 16, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented Apr 16, 2021

Features:

  • Adds new google.protobuf.Any extension field to Contractinfo as extension point
  • Replaces "VanillaJsonEncoder" in CLI with custom Proto-Json marshaler to handle Any types proper. This is a breaking change to the CLI json/ yaml output

This PR touched more files as the new Any field required some helper methods and tests for storage and queries.

To use the Extension you would need to register an implementation to the interface types.ContractInfoExtension

interfaceRegistry.RegisterImplementations(
		(*types.ContractInfoExtension)(nil),
		&YourProtobufType{},
               )

@alpe alpe added the extension point Makes the system easier to extend or test label Apr 16, 2021
@alpe alpe requested a review from ethanfrey April 16, 2021 07:47
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.

I would just ask for some docs on usage, as well as clarifying how ValidateBasic works on the ContractExtension (it is handled slightly different in two places, this must be consistent and explicit - either implementation is fine with me, just make it clear)

cmd/wasmd/root.go Show resolved Hide resolved
x/wasm/client/codec/marshaller.go Outdated Show resolved Hide resolved
x/wasm/keeper/genesis_test.go Show resolved Hide resolved
x/wasm/keeper/genesis_test.go Show resolved Hide resolved
x/wasm/keeper/querier_test.go Show resolved Hide resolved
if c.Extension == nil {
return nil
}
e, ok := c.Extension.GetCachedValue().(validatable)
Copy link
Member

Choose a reason for hiding this comment

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

This should definitely be documented (maybe below?)
not just how to register an Any type for the extension, but all variants of this type must implement ValidateBasic()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh, that was not intended. I had the interface implementation mandatory but made it optional later. Good to have you covering me. 😅

x/wasm/types/types.go Show resolved Hide resolved
x/wasm/types/types.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

Merging #492 (e993e09) into master (4b8d024) will increase coverage by 1.34%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #492      +/-   ##
==========================================
+ Coverage   56.97%   58.32%   +1.34%     
==========================================
  Files          41       41              
  Lines        4411     4441      +30     
==========================================
+ Hits         2513     2590      +77     
+ Misses       1689     1638      -51     
- Partials      209      213       +4     
Impacted Files Coverage Δ
x/wasm/client/cli/genesis_msg.go 72.18% <ø> (ø)
x/wasm/client/cli/query.go 0.00% <0.00%> (ø)
x/wasm/keeper/contract_keeper.go 91.66% <0.00%> (-8.34%) ⬇️
x/wasm/keeper/keeper.go 84.92% <0.00%> (-1.20%) ⬇️
x/wasm/types/types.go 43.33% <84.84%> (+19.40%) ⬆️
x/wasm/keeper/legacy_querier.go 68.75% <100.00%> (ø)
x/wasm/keeper/querier.go 60.10% <100.00%> (+9.28%) ⬆️
x/wasm/types/codec.go 100.00% <100.00%> (+57.89%) ⬆️
... and 1 more

@alpe alpe merged commit d90bf6e into master Apr 16, 2021
@alpe alpe deleted the extend_contract_info branch April 16, 2021 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension point Makes the system easier to extend or test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants