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

Jump to bottom

Add DistributionQuery::DelegatorWithdrawAddress #442

Merged
merged 1 commit into from
Jul 3, 2023
Merged

Add DistributionQuery::DelegatorWithdrawAddress #442

Show file tree
Hide file tree
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
25 changes: 19 additions & 6 deletions types/queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,13 @@ func ToQuerierResult(response []byte, err error) QuerierResult {
// QueryRequest is an rust enum and only (exactly) one of the fields should be set
// Should we do a cleaner approach in Go? (type/data?)
type QueryRequest struct {
Bank *BankQuery `json:"bank,omitempty"`
Custom json.RawMessage `json:"custom,omitempty"`
IBC *IBCQuery `json:"ibc,omitempty"`
Staking *StakingQuery `json:"staking,omitempty"`
Stargate *StargateQuery `json:"stargate,omitempty"`
Wasm *WasmQuery `json:"wasm,omitempty"`
Bank *BankQuery `json:"bank,omitempty"`
Custom json.RawMessage `json:"custom,omitempty"`
IBC *IBCQuery `json:"ibc,omitempty"`
Staking *StakingQuery `json:"staking,omitempty"`
Distribution *DistributionQuery `json:"distribution,omitempty"`
Stargate *StargateQuery `json:"stargate,omitempty"`
Wasm *WasmQuery `json:"wasm,omitempty"`
}

type BankQuery struct {
Expand Down Expand Up @@ -319,6 +320,18 @@ type Delegation struct {
Amount Coin `json:"amount"`
}

type DistributionQuery struct {
DelegatorWithdrawAddress *DelegatorWithdrawAddressQuery `json:"delegator_withdraw_address,omitempty"`
}

type DelegatorWithdrawAddressQuery struct {
DelegatorAddress string `json:"delegator_address"`
}

type DelegatorWithdrawAddressResponse struct {
WithdrawAddress string `json:"withdraw_address"`
Copy link
Member

Choose a reason for hiding this comment

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

I just realize in cosmwasm (Rust) this should be Addr instead of String because we get a checked value from wasmd here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that case we cannot have the Default derive for the DelegatorWithdrawAddressResponse and therefore also not the QueryResponseType impl. Is that alright? I think we need to add a constructor function then, if we want to mock it in multi-test.

Copy link
Member

Choose a reason for hiding this comment

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

Uff, yeah, this problem again. We hit the exact same case with ContractInfoResponse. In essence: in normale cases a contract never constructs this and just consumes it. Just for testing frameworks we need a constructor outside of cosmwasm-std which means we need to either have Default or give up #[non_exhaustive]. Having Default allows constructing invalid instances. I don't like this situation but don't have a solution.

Happy for any solution suggestion to this general problem but let's not block the release on it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose in this case we could just drop the non_exhaustive, as I do not think another field will be added to this response in particular.
I have some ideas on how to solve this in general:

  • keep separate Response structs in multi-test (as they only need to deserialize to the same), but then we have two separate ones, which is very annoying.
  • add a public function to create the response, but with #[doc(hidden)]. This has similar drawbacks as the Default impl, except that we can demand parameters and clearly mark it as not for "normal" usage.

Copy link
Member

Choose a reason for hiding this comment

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

The second option means a non-stable constructor which I guess is fine. The other option between the two is a stable constructor which sets the newly added fields to some default value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, a non-stable constructor or default values if the added fields allow for one, but realistically non-stable constructor should be fine if we hide it and clearly document it to be non-stable.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add those constructors for all query responses? Then we can remove Default here and make it Addr. I think we should favour the production use case over test framework convenience. And having a validated Addr save a call into the chain.

}

// DelegationResponse is the expected response to DelegationsQuery
type DelegationResponse struct {
Delegation *FullDelegation `json:"delegation,omitempty"`
Expand Down
23 changes: 23 additions & 0 deletions types/queries_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,29 @@ func TestContractInfoResponseSerialization(t *testing.T) {
}, res)
}

func TestDistributionQuerySerialization(t *testing.T) {
var err error

// Deserialization
document := []byte(`{"delegator_withdraw_address":{"delegator_address":"jane"}}`)
var query DistributionQuery
err = json.Unmarshal(document, &query)
require.NoError(t, err)
require.Equal(t, query, DistributionQuery{
DelegatorWithdrawAddress: &DelegatorWithdrawAddressQuery{
DelegatorAddress: "jane",
},
})

// Serialization
res := DelegatorWithdrawAddressResponse{
WithdrawAddress: "jane",
}
serialized, err := json.Marshal(res)
require.NoError(t, err)
require.Equal(t, string(serialized), `{"withdraw_address":"jane"}`)
}

func TestCodeInfoResponseSerialization(t *testing.T) {
// Deserializaton
document := []byte(`{"code_id":67,"creator":"jane","checksum":"f7bb7b18fb01bbf425cf4ed2cd4b7fb26a019a7fc75a4dc87e8a0b768c501f00"}`)
Expand Down