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

cosmwasm_std: use BTreeMap in testing mocks #1778

Closed
wants to merge 1 commit into from
Closed

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Jul 14, 2023

Avoid dependency on std in testing module by replacing usage of
HashMap by BTreeMap. The HashMap hasn’t been used in public API other
than DistributionQuerier::new method. While this breaks API, the
method hasn’t been included in stable cosmwasm-std release yet thus
the change is semver-compliant.

An alternative approach would be to change DistributionQuerier::new to
take iterator of pairs. It’d probably be somewhat cleaner even if
it’d cause some unnecessary allocations when constructing the object.

With this change it’s going to be possible to make cosmwasm-std
support no_std in entirely backwards-compatible way (even if someone
builds with no-default-features).

Avoid dependency on std in testing module by replacing usage of
HashMap by BTreeMap.  The HashMap hasn’t been used in public API other
than DistributionQuerier::new method.  While this breaks API, the
method hasn’t been included in stable cosmwasm-std release yet thus
the change is semver-compliant.

An alternative approach would be to change DistributionQuerier::new to
take iterator of pairs.  It’d probably be somewhat cleaner even if
it’d cause some unnecessary allocations when constructing the object.

With this change it’s going to be possible to make cosmwasm-std
support no_std in entirely backwards-compatible way (even if someone
builds with no-default-features).
@mina86
Copy link
Contributor Author

mina86 commented Jul 14, 2023

With this change it’s going to be possible to make cosmwasm-std support no_std in entirely backwards-compatible way (even if someone builds with no-default-features).

I haven’t fully verified this, but I’m fairly certain this is true. There will essentially be no need for std cargo feature¹ if testing module doesn’t use HashMap in public APIs. DistributionQuerier::new is the only offending interface and to be honest I think it should actually be changed into

    pub fn new<T>(withdraw_addresses: T) -> Self
    where
        T: core::iter::IntoIterator<Item = (String, String)>
    {
        /* ... */
    }

¹ Kind of. There’s still one test that will depend on std, but tests don’t influence API compatibility.

@chipshort
Copy link
Collaborator

Unfortunately, we already released 1.3 with the HashMap before seeing this PR, so this is a breaking change after all. However, 2.0 is planned, so we can do the change there.

I agree with the IntoIterator proposal.

@chipshort chipshort added this to the 2.0.0 milestone Jul 20, 2023
@mina86 mina86 closed this Jul 24, 2023
@mina86 mina86 deleted the c branch July 24, 2023 01:37
@webmaster128
Copy link
Member

I agree with the IntoIterator proposal.

@chipshort could you open a ticket for that for 2.0?

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.

3 participants