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 SupplyResponse::new #1552

Merged
merged 4 commits into from Dec 21, 2022
Merged

Add SupplyResponse::new #1552

merged 4 commits into from Dec 21, 2022

Conversation

ghost
Copy link

@ghost ghost commented Dec 20, 2022

Pretty much self explanatory, without this, consumers of cosmwasm-std can't construct new SupplyResponse structs

@webmaster128
Copy link
Member

Hmm, this is a problem. We have this in a different place as well. The moment this constructor is added, we cannot add fields to the response type anymore. And this is the reason there is #[non_exhaustive] on SupplyResponse.

Where do you need contructing a SupplyResponse? Is this for test code?

@ghost
Copy link
Author

ghost commented Dec 20, 2022

Yes exactly, just for testing. You can take a look at these PRs if you're interested

@webmaster128 webmaster128 changed the title supplyresponse::new Add SupplyResponse::new Dec 21, 2022
@webmaster128 webmaster128 merged commit 2bd030a into CosmWasm:main Dec 21, 2022
@ghost ghost deleted the arb/supplyresponse branch December 21, 2022 22:36
@larry0x
Copy link
Contributor

larry0x commented Dec 31, 2022

The moment this constructor is added, we cannot add fields to the response type anymore.

If backward compatibility is the concern, how about this solution. Suppose I have this struct:

#[non_exhaustive]
struct MyStruct {
    a: String,
    b: String,
    c: String,
}

impl MyStruct {
    pub fn new(a: String, b: String, c: String) -> Self {
        Self { a, b, c }
    }
}

Let's say in cosmwasm v1.2 we want to add a new field d to this. We can then do this:

#[non_exhaustive]
struct MyStruct {
    a: String,
    b: String,
    c: String,
    #[cfg(feature = "cosmwasm_1_2")]
    d: String,
}

impl MyStruct {
    #[cfg(not(feature = "cosmwasm_1_2"))]
    pub fn new(a: String, b: String, c: String) -> Self {
        Self { a, b, c }
    }

    #[cfg(feature = "cosmwasm_1_2")]
    pub fn new(a: String, b: String, c: String, d: String) -> Self {
        Self { a, b, c, d }
    }
}

This way, anyone who doesn't enable the cosmwasm_1_2 feature will get the old function signature (without d), while everyone who does enables the feature gets the new function signature (with d).

@webmaster128
Copy link
Member

Unfortunately this conflicts with the idea that Rust features must be additive. In the given example, a MyStruct::new(one, two, three) call fails to compile once the feature is added.

We have such problems in the codebase around the ibc3 feature but they will be removed as soon as we get the chance.

@webmaster128
Copy link
Member

What if instead we implement Default for those types. Then host code (multi-test, cw-sdk) can construct the types like this:

fn construct_me(testing_amount: Coin) -> SupplyResponse {
    let mut res = SupplyResponse::default();
    res.amount = testing_amount;
    res
}

@larry0x
Copy link
Contributor

larry0x commented Jan 2, 2023

What if instead we implement Default for those types. Then host code (multi-test, cw-sdk) can construct the types like this:

fn construct_me(testing_amount: Coin) -> SupplyResponse {
    let mut res = SupplyResponse::default();
    res.amount = testing_amount;
    res
}

Sounds like a good solution

@webmaster128
Copy link
Member

Could you check out the work in #1560 for an implementation of that approach?

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 this pull request may close these issues.

2 participants