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.

Sign up for GitHub

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

Update WasmMsg types #213

Closed
wants to merge 1 commit into from
Closed

Update WasmMsg types #213

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions types/msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,8 @@ type InstantiateMsg struct {
Send Coins `json:"send"`
// Label is optional metadata to be stored with a contract instance.
Label string `json:"label"`
// Admin (optional) may be set here to allow future migrations from this address
Admin string `json:"admin,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this a *string to match the Rust type Option<String>

Copy link
Member Author

Choose a reason for hiding this comment

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

Label is also optional. *string is kind of ugly in Go, so I avoid it. Go standard is to treat "" as nil. Unless an empty string and nil have different actions (eg. a patch for a profile may use this, so *description == "" clears the description while description == nil leaves the description unchanged)

We try to stick with the most idiomatic structs in each language. And there is no valid "" for Admin (it must be a long bech32 string)

Copy link
Member

Choose a reason for hiding this comment

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

The difference for Label is that we decided to make it a simple String in Rust as well.

I think using plain string works in this particular case, but only we throw away the empty/nil differentiation and because we only use this from Rust -> Go.

It would be nice if we could establish a type pattern that is consistent over all places. With omitempty we send undefined from Go to Rust where string or null is expected. This probably fails when decoding. Also the Go side should error for unexpected JSON values (as this is a JSON interface):

  • null and bech32 is valid
  • Empty string is an error
  • All other JSON types are an error
  • Unset is an error

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice if we could establish a type pattern that is consistent over all places. With omitempty we send undefined from Go to Rust where string or null is expected. This probably fails when decoding.

No, I have tested this quite a bit last year. Option<T> happy converts null and a missing field to None.
I had issues when Go converted an empty array to null and then we tried to parse into Vec<T>. In that case I overrode GO serialization to produce [] for empty arrays and keep the Rust type sane.

I would like @alpe feedback here, as this is really about keeping canonical Go types. In generally if there is no strong reason to differentiate between "zero-values" and "null", you treat them the same in Go. There is no simple way to differentiate null from undefined (missing field), even if you use *string.

Copy link
Contributor

Choose a reason for hiding this comment

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

I need to second Ethan in this discussion. The*string types are very uncommon in Go and only when nil and empty string are treated differently. omitempty describes it well as optional field (for any Go developer).

For this field we use the string type already in other places. For example: https://github.com/CosmWasm/wasmd/blob/master/proto/cosmwasm/wasm/v1beta1/tx.proto#L55

}

// MigrateMsg will migrate an existing contract from it's current wasm code (logic)
Expand Down