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

Remove json type cast for contract msgs #520

Merged
merged 1 commit into from
May 25, 2021
Merged

Remove json type cast for contract msgs #520

merged 1 commit into from
May 25, 2021

Conversation

alpe
Copy link
Contributor

@alpe alpe commented May 21, 2021

Removes the cast type as it can cause some issues when unmarshaling from base64 encoded type. Better let the standardlib handle this.

@alpe alpe requested a review from ethanfrey May 21, 2021 09:15
@alpe alpe force-pushed the contract_msg_cast branch from a8228e6 to 98431c6 Compare May 21, 2021 12:06
@codecov
Copy link

codecov bot commented May 21, 2021

Codecov Report

Merging #520 (98431c6) into master (4631658) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #520      +/-   ##
==========================================
+ Coverage   58.68%   58.71%   +0.03%     
==========================================
  Files          43       43              
  Lines        4504     4508       +4     
==========================================
+ Hits         2643     2647       +4     
  Misses       1644     1644              
  Partials      217      217              
Impacted Files Coverage Δ
x/wasm/types/proposal.go 83.03% <100.00%> (+0.30%) ⬆️

@ethanfrey
Copy link
Member

This cast was there for historical reasons. Pre-stargate, we had annoying strings like msg: "eyJmb28iOiJiYXIifQ==" in the output and we wanted msg: {"foo":"bar"}. Since both CLI and CosmJS queries went through the REST server which did the amino->JSON conversion in Go, this was needed.

@webmaster128 can correct me here if I am wrong, but I believe this change will have no impact on the cosmwasm-stargate package. It parsed protobuf byte field and manually converts it to a JSON object inside CosmJS. The GO casttype directive will have no impact.

The only other concern I have is the CLI query output. Can you check if this changes, and if it does, maybe a way to properly format/present this for such queries. Or will this only affect eg. block explorers?

if !json.Valid(p.InitMsg) is a nice check to provide better validity checks.

If there are no issues breaking the output to clients, I am very supportive of this change.

@webmaster128
Copy link
Member

webmaster128 commented May 25, 2021

The gogoproto annotation does not change non-gogoproto code generation at all. So this is safe from my point of view.

The annotation was invalid and it is good that it is removed because the type json.RawMessage

is a raw encoded JSON value.

but we have no guerantee at all that the bytes sent are valid JSON. I.e. a simple cast is not good enough. If we needed that type, we had to do a validation followed by a cast.

@alpe
Copy link
Contributor Author

alpe commented May 25, 2021

The only other concern I have is the CLI query output. Can you check if this changes, and if it does, maybe a way to properly format/present this for such queries. Or will this only affect eg. block explorers?

The CLI is using the GRPC query endpoint only. We store the init message with the Code history entry as raw json entry. The CLI query for this works as before:

wasmd q wasm contract-history wasm10pyejy66429refv3g35g2t7am0was7yajwflk7 -o json | jq

{
  "entries": [
    {
      "operation": "CONTRACT_CODE_HISTORY_OPERATION_TYPE_INIT",
      "code_id": "2",
      "msg": {
        "verifier": "wasm1alz4eapgf0rq3ezf3xge9pz7uhkpdyu2mmrrup",
        "beneficiary": "wasm18t98u4xqa8zaq3ewfeq0u7yeee8cqc2sq9keek"
      }
    },
    {
      "operation": "CONTRACT_CODE_HISTORY_OPERATION_TYPE_MIGRATE",
      "code_id": "3",
      "msg": {
        "payout": "wasm18t98u4xqa8zaq3ewfeq0u7yeee8cqc2sq9keek"
      }
    }
  ],
  "pagination": {}
}

I have not tested the query with the legacy REST endpoint but I would not expect any differences as the history model has not changed.
I don't know how block explorer read the data but if they use the proto model to map a TX back to json representations then the init message is likely base64 encoded now

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.

Thanks for responding to my question. Good answer and happy to see this merged in.

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.

3 participants