Migrate to v0.11 #104

maurolacy · 2020-10-02T09:00:39Z

WIP for #96.

ethanfrey

Nice work, and quite a bit of cleanup here.

I like your usage of very clear error cases in subkeys. It is quite good in atomic swap as well, but I think there are still too many user-facing strings being created in the code, not in the error type.

I think it is best practice to only return data related to the error inside the ContractError type, and all formatting to make it reasonable (making use of the metadata) is done in the #[error] text. This also gives a very clear overview for anyone trying to adjust the messages (or trace a bug)

contracts/cw1-subkeys/src/contract.rs

ethanfrey · 2020-10-03T18:57:40Z

contracts/cw1-subkeys/src/contract.rs

        );

        // And then cannot (not enough funds anymore)
        let res = handle(&mut deps, env, handle_msg.clone());
        match res.unwrap_err() {
-            StdError::Underflow { .. } => {}
+            ContractError::Std(StdError::Underflow { .. }) => {}


Good example to show how this is compatible with StdErrors

But since we are are creating custom errors everywhere, maybe we want to map this.
Either where it is generated like .map_err(|e| ...). Or do something like:

impl From<cw1_whitelist::ContractError> for ContractError fn from(err: cw1_whitelist::ContractError) -> Self { match err { cw1_whitelist::ContractError::Std(error) => ContractError::Std(error), cw1_whitelist::ContractError::Unauthorized {} => ContractError::Unauthorized {}, } } }

But mapping StdError::Underflow{} => ContractError::InsufficientAllowance{}?

Although the underflow message does show you the amounts, which is nice and shouldn't be lost.

This is an idea, take it or leave it.

I would leave it as it is. Besides the extra boilerplate, it's pretty clear what's being done.

contracts/cw1-subkeys/src/error.rs

contracts/cw1-subkeys/src/state.rs

contracts/cw20-atomic-swap/src/contract.rs

ethanfrey · 2020-10-03T19:15:44Z

contracts/cw20-escrow/src/contract.rs

@@ -192,17 +192,17 @@ pub fn try_refund<S: Storage, A: Api, Q: Querier>(
        Err(StdError::unauthorized())
    } else {
        // we delete the escrow (TODO: expose this in Bucket for simpler API)
-        prefixed(PREFIX_ESCROW, &mut deps.storage).remove(id.as_bytes());
+        prefixed(&mut deps.storage, PREFIX_ESCROW).remove(id.as_bytes());


Note, this is now in Bucket.remove you can use that now.

Will change those in another branch / PR.

ethanfrey · 2020-10-03T19:16:08Z

contracts/cw20-escrow/src/contract.rs

@@ -161,17 +161,17 @@ pub fn try_approve<S: Storage, A: Api, Q: Querier>(
        Err(StdError::generic_err("escrow expired"))
    } else {
        // we delete the escrow (TODO: expose this in Bucket for simpler API)
-        prefixed(PREFIX_ESCROW, &mut deps.storage).remove(id.as_bytes());
+        prefixed(&mut deps.storage, PREFIX_ESCROW).remove(id.as_bytes());


And here... time to update

maurolacy · 2020-10-04T08:52:24Z

It is quite good in atomic swap as well, but I think there are still too many user-facing strings being created in the code, not in the error type.

You're right, I see you point: The very idea of custom errors is to move the messages to the error definition; and in any case just pass parameters. And that, only if justified.

ethanfrey · 2020-10-04T10:44:30Z

I think there is a lot of work migrating to proper custom errors, and a lot of this can be done in parallel to other 0.11 upgrade stuff. I am preparing some larger changes to bucket (and adding an indexed bucket) for 0.11-alpha4.

Now that you have fully migrated the error messages in a few contracts, and we can get some clarity there on what makes sense to us both, it would be great if you could finish up the other contracts with minimal changes. Either somehow asking clippy to ignore the deprecation warning for a minute, or just using a StdError + Unauthorized minimal wrapper, like in cw1-whitelist, to the other contracts you have not yet ported.

Oh, and rebasing/merging on master to resolve all conflicts (I think mainly with the 0.2.2 release and version changes?)

Then please take out of draft mode, so we can merge this in, and you can keep updating the error codes on one branch, while I can update the Bucket refactor on another branch. I hope to do the Bucket refactor monday or tuesday

Rename log to attr/attributes

Change FullDelegation accumulated_rewards to Vec<Coin>

maurolacy · 2020-10-04T16:18:15Z

Now that you have fully migrated the error messages in a few contracts, and we can get some clarity there on what makes sense to us both, it would be great if you could finish up the other contracts with minimal changes. Either somehow asking clippy to ignore the deprecation warning for a minute, or just using a StdError + Unauthorized minimal wrapper, like in cw1-whitelist, to the other contracts you have not yet ported.

Turns out is not so easy to do that, because once I change to ContractError, that forces me to change all the branches of the match in handle(), and so on.

What I can do is leave the warnings (or revert to using v0.10.1 in the remaining contracts) for now, and focus on the rebase from master and other small changes, in order to remove Draft mode. So we can merge the work so far.

Update: Tried to revert remaining contract (staking) to 0.10.1, but that's not possible due to "library"-type dependencies. So I guess forward is the simple path. I'll try to finish migration tomorrow, to a point that it can be merged without errors / warnings.

…d to 0.11)

maurolacy · 2020-10-04T21:42:46Z

OK, migrated all but the two new contracts (cw3-fixed-multisig and cw721-nft).

Also rebased from master. Will remove Draft, so you can review and merge this if you want. And, I can migrate the other two contracts in a different branch / PR.

maurolacy · 2020-10-04T22:15:43Z

It's probably a good idea to increase the version of the packages to 0.2.3, before publishing.

ethanfrey · 2020-10-05T09:26:34Z

It's probably a good idea to increase the version of the packages to 0.2.3, before publishing.

Good point, but I don't intend to publish anything until 0.3.0, with 0.11 support. As 0.10 -> 0.11 is breaking.

Thank you for finishing this up to a usable point. I will review and merge and we can both work on this as needed (I would just mess with the bucket and query stuff from my open PRs on cosmwasm with the next 0.11-alpha, and leave all the error work to you to ensure we don't step on toes)

ethanfrey

Good stuff here, and thanks for getting it stable.

I will merge this in. Left a few comments along the way that may be useful for the next segment of your error-refactoring PR.

contracts/cw20-escrow/src/error.rs

contracts/cw20-staking/src/contract.rs

ethanfrey · 2020-10-05T09:32:23Z

contracts/cw20-staking/src/error.rs

+}
+
+impl From<cw20_base::ContractError> for ContractError {
+    fn from(err: cw20_base::ContractError) -> Self {


Nice extension.

I did try to figure out how to "extend" enums in rust and it is not possible.
I wonder if we could simplify this one day with codegen? But I guess it is only a few minutes to write such code now.

Yeah, these cases made me search for "rust inheritance" and the like... Traits are fine and dandy, but for real abstraction there's nothing like subtypes (interface inheritance) IMO.

Come to think of it, this is probably more a case requiring or benefiting from "plain old" (implementation) inheritance, rather than interface inheritance.

ethanfrey · 2020-10-05T09:34:26Z

contracts/cw20-staking/src/error.rs

+    NotInValidatorSet(String),
+
+    #[error("Different denominations in bonds: '{0}' vs. '{1}'")]
+    DifferentBondDenom(String, String),


Question: would it make sense to have these named (struct) variants rather than tuple variants, where there is more than one argument?

I think the generated code is the same, you just then use DifferentBondDenom{contract: String, message: String} or something like that to make it clear which side is which.

I think it makes sense, yes. I'll change it for clarity.

ethanfrey · 2020-10-05T09:35:27Z

contracts/cw3-fixed-multisig/Cargo.toml

-cw0 = { path = "../../packages/cw0", version = "0.2.2" }
-cw2 = { path = "../../packages/cw2", version = "0.2.2" }
-cw3 = { path = "../../packages/cw3", version = "0.2.2" }
+cw0 = { version = "0.2.2" }


Ah, interesting way to do this... as long as no one else imports this, we can hold it back to the older package versions. Nice idea!

Yeah, that took a while... :-) happy that I made it compile (and then mergeable).

maurolacy requested review from webmaster128, ethanfrey and orkunkl October 2, 2020 09:00

maurolacy self-assigned this Oct 2, 2020

ethanfrey reviewed Oct 3, 2020

View reviewed changes

maurolacy added 16 commits October 4, 2020 16:45

Migrate contracts to >0.10.1 (0.11)

87aaddc

Rename log to attr/attributes

Migrate to 0.11

bfbd3fa

Change FullDelegation accumulated_rewards to Vec<Coin>

Update dependencies to 0.11.0-alpha2

f11ff50

Change to new bucket parameters order

bd8c8b2

(Manually) fix rebase from master

0551997

Update deps to v0.11.0-alpha3

b6c116e

Migrate cw1-whitelist to ContractError

a1e8c26

Migrate cw1-subkeys to thiserror / custom ContractError

c3bbfc2

Update Cargo.lock

4b47e56

Migrate cw20-atomic-swap to thiserror / ContractError

ccf4f9a

Fix Underflow error match

9bd5bda

Fix Cargo.lock

1c8bbb3

Better / consolidated error types

2b0f72d

Reverting NotFound to NoAllowance

a89ffee

Custom errors specialization

b31a5f8

Migrate cw20-base to thiserror / ContractError

0ffdab5

maurolacy added 4 commits October 4, 2020 23:05

Migrate cw20-escrow to thiserror / ContractError

9685727

Migrate cw20-staking to thiserror / ContractError

678dba9

Upstream versions of cw* pkgs for cw3-fixed-multisig (not yet migrate…

c4389eb

…d to 0.11)

Upstream versions of cw* pkgs for cw721-nft (not yet migrated to 0.11)

8e48aaa

maurolacy force-pushed the migrate-to-v0.11 branch from 2bd9776 to 8e48aaa Compare October 4, 2020 21:38

maurolacy marked this pull request as ready for review October 4, 2020 21:43

ethanfrey approved these changes Oct 5, 2020

View reviewed changes

ethanfrey merged commit 17b5781 into master Oct 5, 2020

ethanfrey deleted the migrate-to-v0.11 branch October 5, 2020 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to v0.11 #104

Migrate to v0.11 #104

maurolacy commented Oct 2, 2020

ethanfrey left a comment

ethanfrey Oct 3, 2020

ethanfrey Oct 3, 2020

ethanfrey Oct 3, 2020

maurolacy Oct 4, 2020 •

edited

Loading

ethanfrey Oct 3, 2020

maurolacy Oct 4, 2020

ethanfrey Oct 3, 2020

maurolacy commented Oct 4, 2020 •

edited

Loading

ethanfrey commented Oct 4, 2020 •

edited

Loading

maurolacy commented Oct 4, 2020 •

edited

Loading

maurolacy commented Oct 4, 2020 •

edited

Loading

maurolacy commented Oct 4, 2020 •

edited

Loading

ethanfrey commented Oct 5, 2020

ethanfrey left a comment

ethanfrey Oct 5, 2020

maurolacy Oct 5, 2020 •

edited

Loading

maurolacy Oct 5, 2020 •

edited

Loading

ethanfrey Oct 5, 2020

maurolacy Oct 5, 2020

ethanfrey Oct 5, 2020

maurolacy Oct 5, 2020

Migrate to v0.11 #104

Migrate to v0.11 #104

Conversation

maurolacy commented Oct 2, 2020

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Oct 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy commented Oct 4, 2020 • edited Loading

ethanfrey commented Oct 4, 2020 • edited Loading

maurolacy commented Oct 4, 2020 • edited Loading

maurolacy commented Oct 4, 2020 • edited Loading

maurolacy commented Oct 4, 2020 • edited Loading

ethanfrey commented Oct 5, 2020

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

maurolacy Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Oct 4, 2020 •

edited

Loading

maurolacy commented Oct 4, 2020 •

edited

Loading

ethanfrey commented Oct 4, 2020 •

edited

Loading

maurolacy commented Oct 4, 2020 •

edited

Loading

maurolacy commented Oct 4, 2020 •

edited

Loading

maurolacy commented Oct 4, 2020 •

edited

Loading

maurolacy Oct 5, 2020 •

edited

Loading

maurolacy Oct 5, 2020 •

edited

Loading