You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
Assign a default value in the old construtor. The testing framework can change it after creation using a mutable instance.
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)
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)
The text was updated successfully, but these errors were encountered:
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.
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.
See also #1558. I'd like to challenge my original argument
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:Reply::new(id, result)
andReply::new_with_gas_used(id, result, gas_used)
So I suggest
AllDenomMetadataResponse
/DenomMetadataResponse
/QueryResponseType
(1.3) (Query Response Constructors #1766)DelegatorWithdrawAddressResponse
and turnwithdraw_address
intoAddr
(1.3) (Query Response Constructors #1766)ContractInfoResponse
and turncreator
/admin
intoAddr
(2.0)CodeInfoResponse
and turncreator
intoAddr
(2.0)AllBalanceResponse
/BalanceResponse
/SupplyResponse
as they are just pointless (2.0)The text was updated successfully, but these errors were encountered: