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

Optional fields deserialization in MsgEditSideChainValidator #360

Open
4 tasks
MarcinKustraCopper opened this issue Nov 13, 2023 · 0 comments
Open
4 tasks

Comments

@MarcinKustraCopper
Copy link

Summary of Bug

Recently I've been trying to integrate MsgEditSideChainValidator via Javascript SDK and Amino codec and I found interesting case with optional fields. E.g when I want to update Side Chain Validator:

type MsgEditSideChainValidator struct {
	Description   Description    `json:"description"`
	ValidatorAddr sdk.ValAddress `json:"address"`

	// We pass a reference to the new commission rate as it's not mandatory to
	// update. If not updated, the deserialized rate will be zero with no way to
	// distinguish if an update was intended.
	//
	// REF: #2373
	CommissionRate *sdk.Dec `json:"commission_rate"`

	SideChainId string `json:"side_chain_id"`
	// for SideFeeAddr, we do not update the values if they are not provided.
	SideFeeAddr []byte `json:"side_fee_addr"`

	SideConsAddr []byte `json:"side_cons_addr,omitempty"`
}

with only e.g. updated website, sending such amino-encoded payload:

{"type":"cosmos-sdk/MsgEditSideChainValidatorWithVoteAddr","value":{"address":"bva1...","description":{"details":"[do-not-modify]","identity":"[do-not-modify]","moniker":"[do-not-modify]","website":"foo.bar"},"side_chain_id":"chainId"}}

In this payload fields that I do not want to update were filled with nulls, yet they were skipped in JSON repr. Such payload also is used for signing purposes.
However, decoding this message on bnc-cosmos-sdk side and getting sign bytes yields completely different shape:

{"type":"cosmos-sdk/MsgEditSideChainValidatorWithVoteAddr","value":{"address":"bva1...","commission_rate":null,"description":{"details":"foo.bar","identity":"[do-not-modify]","moniker":"[do-not-modify]","website":"[do-not-modify]"},"side_chain_id":"chainId","side_fee_addr":null,"side_vote_addr":null}}

yielding in completely different sign data bytes.

Steps to Reproduce

As above.

Expected behaviour

Unified behaviour between js and cosmos SDKs.
I think that Golang SDK should drop nullable fields, since it is more flexible to add new fields without breaking compatibility with previous JS SDKs.
I submitted PR as part of this issue.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
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 a pull request may close this issue.

1 participant