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

Add constructors to query responses #1765

Closed
5 tasks done
webmaster128 opened this issue Jul 5, 2023 · 3 comments · Fixed by #1883
Closed
5 tasks done

Add constructors to query responses #1765

webmaster128 opened this issue Jul 5, 2023 · 3 comments · Fixed by #1883
Milestone

Comments

@webmaster128
Copy link
Member

webmaster128 commented Jul 5, 2023

See also #1558. I'd like to challenge my original argument

Simply adding a new function is not a satisfactory solution, as @webmaster128 #1552 (comment), we would not be able to add new fields to the response type without changing the function signature.

That we are seeing now is worse than incomplete or breaking constructors that only affect testing frameworks. By enforcing a Default implementation for the query responses we cannot use types that have no Default implementation for good reason. E.g. we can't use Addr, forcing the user to validate data again that comes form a trusted source or rely on unsafe string comparisons. This will become more visible by #1669.

I don't think we should allow drawback for contract developers and production code to make writing testing frameworks convenient.

What we can do instead is: create constructors that fill all the fields. They can potentially be #[doc(hidden)] as shown in https://github.com/CosmWasm/cosmwasm/pull/1552/files. Then when new fields come in, we have three options:

  1. Assign a default value in the old construtor. The testing framework can change it after creation using a mutable instance.
  2. Create a second additional constructor that adds the new field (such as Reply::new(id, result) and Reply::new_with_gas_used(id, result, gas_used)
  3. Break the constructor and just add the field

So I suggest

  • Remove Default implementation from AllDenomMetadataResponse/DenomMetadataResponse/QueryResponseType (1.3) (Query Response Constructors #1766)
  • Remove Default implementation from DelegatorWithdrawAddressResponse and turn withdraw_address into Addr (1.3) (Query Response Constructors #1766)
  • Remove Default implementation from ContractInfoResponse and turn creator/admin into Addr (2.0)
  • Remove Default implementation from CodeInfoResponse and turn creator into Addr (2.0)
  • Remove Default implementation from AllBalanceResponse/BalanceResponse/SupplyResponse as they are just pointless (2.0)
@webmaster128 webmaster128 added this to the 1.3.0 milestone Jul 5, 2023
@chipshort
Copy link
Collaborator

As mentioned already, I agree with this.
For option 3, we probably want to be able to break these constructors even in minor releases, so we should clearly mark that in their doc comments.
Just want to note that for larry's use case in #1558, we'd need to either keep these semver-stable or he'd have to pin the exact std-version. Otherwise, we'd potentially break cw-sdk.

@webmaster128
Copy link
Member Author

webmaster128 commented Jul 5, 2023

Just want to note that for larry's use case in #1558, we'd need to either keep these semver-stable or he'd have to pin the exact std-version. Otherwise, we'd potentially break cw-sdk.

A pragmatic approach would be to not break those things in patch releases and use the tilde notation for the dependencies. Going explicitly from minor to minor is probably a good idea anyways if you integrate CosmWasm in a host environment.

@webmaster128
Copy link
Member Author

ping @larry0x btw

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.

2 participants