Update cosmwasm-std #260

yihuang · 2021-04-02T02:15:13Z

Maybe merge after the cosmwasm-std version released.

ethanfrey · 2021-04-06T20:13:17Z

Thanks for this initiative. There are a few more changes, and let's wait til a 0.14.0-beta2 is out to finalize this.

ethanfrey · 2021-04-07T20:27:36Z

https://github.com/CosmWasm/cosmwasm/releases/tag/v0.14.0-beta2 is now out (inspired by your work to cut this now).

No pressure, but feel free to upgrade to that and I'd be happy to merge.
Only remaining (major) plans for 0.14.0 is some refactoring of the IBC bindings, but everything else should be pretty stable now.

yihuang · 2021-04-08T07:42:40Z

https://github.com/CosmWasm/cosmwasm/releases/tag/v0.14.0-beta2 is now out (inspired by your work to cut this now).

No pressure, but feel free to upgrade to that and I'd be happy to merge.
Only remaining (major) plans for 0.14.0 is some refactoring of the IBC bindings, but everything else should be pretty stable now.

There are many changes needed in multi-test for recent address change, I'm not sure when to use Addr::unchecked or addr_validate.

maurolacy · 2021-04-08T07:57:31Z

There are many changes needed in multi-test for recent address change, I'm not sure when to use Addr::unchecked or addr_validate.

This comment https://github.com/CosmWasm/cosmwasm/blob/main/packages/std/src/addresses.rs#L30:L44 may be of help.

yihuang · 2021-04-08T07:59:40Z

There are many changes needed in multi-test for recent address change, I'm not sure when to use Addr::unchecked or addr_validate.

This comment https://github.com/CosmWasm/cosmwasm/blob/main/packages/std/src/addresses.rs#L30:L44 may be of help.

Since multi-test is a test library, i suppose it's free to use Addr:unchecked as needed?

maurolacy · 2021-04-08T08:04:30Z

There are many changes needed in multi-test for recent address change, I'm not sure when to use Addr::unchecked or addr_validate.

This comment https://github.com/CosmWasm/cosmwasm/blob/main/packages/std/src/addresses.rs#L30:L44 may be of help.

Since multi-test is a test library, i suppose it's free to use Addr:unchecked as needed?

I guess it depends on the particular case. Basically, use Addr::unchecked to generate a dummy Addr from a string in test code. Or an Addr in contract code, if it has already been validated. For the other cases, you should use addr_validate() to generate an Addr from a string. Or addr_humanize(), to generate an Addr from a (binary repr) CanonicalAddr.

If you post a specific example / instance, we can discuss it.

maurolacy · 2021-04-08T08:15:23Z

On second thought, I think you're right. If we want the multi-test framework to work generically over addresses, unchecked would be the right choice.

ethanfrey · 2021-04-08T08:15:25Z

I just merged in your cw1155 contract. Many thanks for that.

If you want to rebase and push with latest master and v0.14.0-beta2, I will look at the compile failures in multi-test and maybe can fix them (or at least find a good solution). multi-test is kind of my baby and in beta.

ethanfrey · 2021-04-08T08:31:58Z

My short feedback is in non-test code s/HumanAddr/Addr/ and if we really need to create them, we look at it (and avoid unchecked if possible).

In test code, HumanAddr::from("owner") -> Addr::unchecked("owner")

yihuang · 2021-04-08T08:44:33Z

@ethanfrey @maurolacy I pushed lots of address related changes, it's not complete yet, I guess you can check it out now to see if I'm on the right track 😄

ethanfrey · 2021-04-08T09:04:34Z

Thank you, I will look

ethanfrey

Took a brief look at multi-test.

I added some comments, but I am not sure if I am just making a huge pile of work here. Let's go with your approach using Addr::unchecked as needed here and I can always do another PR later to add validation. Multi-test is only test code, so this is more pedantic.

I will look at the contracts to see use in production code.

ethanfrey · 2021-04-08T09:08:29Z

packages/multi-test/src/app.rs

        match msg {
            WasmMsg::Execute {
                contract_addr,
                msg,
                send,
            } => {
+                let contract_addr = Addr::unchecked(contract_addr);


I would validate here instead of unchecked.

let contract_addr = self.api.addr_validate(&contract_addr)?; should work here, well, except for the String error.

Maybe add App.add_validate(addr: &str) -> Result<Addr, String> which is a small wrapper around the above, then we can use that where needed in production code.

ethanfrey · 2021-04-08T09:10:12Z

packages/multi-test/src/app.rs

@@ -381,9 +378,9 @@ where
                send,
                label: _,
            } => {
-                let contract_addr = self.wasm.register_contract(code_id as usize)?;
+                let contract_addr = Addr::unchecked(self.wasm.register_contract(code_id as usize)?);


register_contract should return Addr.

ethanfrey

Added some feedback.

This is hard to get right, and I only understood the proper usage after discussing with Simon on the PR. We need to update the migration / readme - any suggestions are welcome.

Basically, anything that can be set by external users should be String. This includes anything entering and leaving the contract (query response is parsed by someone else and will not trust the contract by default).

Anything we save to state should be Addr, to ensure it is valid.

We can remove almost all usage of CanonicalAddr. You can keep them for keys (like maybe_canonical), but I may replace that with Addr in the keys in the future.

ethanfrey · 2021-04-08T09:13:24Z

packages/cw0/src/pagination.rs

 }

 // this will set the first key after the provided key, by appending a 0 byte
 pub fn calc_range_start_human(
    api: &dyn Api,
-    start_after: Option<HumanAddr>,
+    start_after: Option<Addr>,


I don't think this is used anywhere anymore. It can be removed.

ethanfrey · 2021-04-08T09:13:32Z

packages/cw1/src/helpers.rs

 /// for working with this.
 ///
 /// If you wish to persist this, convert to Cw1CanonicalContract via .canonical()
 #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
-pub struct Cw1Contract(pub HumanAddr);
+pub struct Cw1Contract(pub Addr);


ethanfrey · 2021-04-08T09:14:12Z

packages/cw1/src/helpers.rs

@@ -42,7 +42,7 @@ pub struct Cw1CanonicalContract(pub CanonicalAddr);
 impl Cw1CanonicalContract {


You can remove this entirely, as I we are moving away from CanonicalContracts.

Fine if you leave this for now to make the PR smaller, and I can take a second PR for that one.

ethanfrey · 2021-04-08T09:14:41Z

packages/cw2/src/lib.rs

@@ -44,7 +42,7 @@ pub fn set_contract_version<T: Into<String>, U: Into<String>>(
 /// if the other contract exists and claims to be a cw20-base contract for example.
 /// (Note: you usually want to require *interfaces* not *implementations* of the
 /// contracts you compose with, so be careful of overuse)
-pub fn query_contract_info<Q: Querier, T: Into<HumanAddr>>(
+pub fn query_contract_info<Q: Querier, T: Into<String>>(


Into<String> 👍

ethanfrey · 2021-04-08T09:15:59Z

packages/cw3/src/query.rs

@@ -31,20 +31,20 @@ pub enum Cw3QueryMsg {
    /// Query the vote made by the given voter on `proposal_id`. This should
    /// return an error if there is no such proposal. It will return a None value
    /// if the proposal exists but the voter did not vote. Returns VoteResponse
-    Vote { proposal_id: u64, voter: HumanAddr },
+    Vote { proposal_id: u64, voter: Addr },


All *Msg types should use String (this is unverified input from users)
All state should use Addr.

By doing so, the compiler should enforce we validate input before saving it

ethanfrey · 2021-04-08T09:16:16Z

packages/cw3/src/query.rs

@@ -168,7 +168,7 @@ pub struct VoteListResponse {
 /// the address of the voter who submitted it
 #[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
 pub struct VoteInfo {
-    pub voter: HumanAddr,
+    pub voter: Addr,


Again String here

ethanfrey · 2021-04-08T09:17:35Z

packages/cw4/src/hook.rs

-        old_weight: Option<u64>,
-        new_weight: Option<u64>,
-    ) -> Self {
+    pub fn new<T: Into<Addr>>(addr: T, old_weight: Option<u64>, new_weight: Option<u64>) -> Self {


Is there anything that really implements Into<Addr>? I think we can just use Addr here (or does this allow &addr as well as addr as args?)

ethanfrey · 2021-04-08T09:18:15Z

packages/cw4/src/hook.rs

@@ -53,10 +49,10 @@ impl MemberChangedHookMsg {
    }

    /// creates a cosmos_msg sending this struct to the named contract
-    pub fn into_cosmos_msg(self, contract_addr: HumanAddr) -> StdResult<CosmosMsg> {
+    pub fn into_cosmos_msg(self, contract_addr: Addr) -> StdResult<CosmosMsg> {


Maybe Into<String> here? So we can use Addr, String, or &str?

ethanfrey · 2021-04-08T09:19:31Z

packages/cw4/src/hook.rs

@@ -10,17 +10,13 @@ use cosmwasm_std::{to_binary, Binary, CosmosMsg, HumanAddr, StdResult, WasmMsg};
 /// old = Some, new = None -> Delete
 #[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
 pub struct MemberDiff {
-    pub key: HumanAddr,
+    pub key: Addr,


Wait, this is a message, not state.
Again, use String.
Addr only in state and env/info from the runtime (already trusted)

ethanfrey · 2021-04-08T09:24:01Z

This may be easier to collaborate if you could make a branch on this repo, so I would push some code and collaborate on this PR (this is a huge cleanup task we don't need to dump entirely on you, but thank you very much for taking it on).
Do you want write access to this repo?

yihuang · 2021-04-08T09:36:08Z

This may be easier to collaborate if you could make a branch on this repo, so I would push some code and collaborate on this PR (this is a huge cleanup task we don't need to dump entirely on you, but thank you very much for taking it on).
Do you want write access to this repo?

I've enabled the option Allow edits by maintainers, I think you can already push to my branch?

ethanfrey · 2021-04-08T09:37:49Z

Okay, I will try that and do a quick Addr -> String in the messages.

ethanfrey · 2021-04-08T09:47:57Z

Okay, I just pushed a commit for cw1-4 to show what I mean. I am off for lunch for an hour or so.

If this makes sense, feel free to continue. If not, I can pick this up later today

maurolacy · 2021-04-12T11:03:39Z

@yihuang , can you take it from here and implement the needed changes (add schema) for cw1155-base to pass CI?

maurolacy · 2021-04-12T11:06:03Z

Besides, I think this will require at least a quick review before merging, to try and spot any errors / omissions.

ethanfrey · 2021-04-12T12:28:10Z

Besides, I think this will require at least a quick review before merging, to try and spot any errors / omissions.

I will do a review on everything but cw1155 for now, and that when @yihuang flags it as ready.
Thanks for doing all this work @maurolacy

yihuang · 2021-04-13T01:54:17Z

@yihuang , can you take it from here and implement the needed changes (add schema) for cw1155-base to pass CI?

it seems has done by @ethanfrey ? 👍

maurolacy · 2021-04-13T08:20:52Z

@yihuang , can you take it from here and implement the needed changes (add schema) for cw1155-base to pass CI?

it seems has done by @ethanfrey ? +1

Yes. We wanted to see the CI solid green. :-)

ethanfrey · 2021-04-13T10:59:04Z

I'm pulling out all AddrRef to &Addr before merging. Otherwise, looking good

webmaster128 · 2021-04-13T14:03:14Z

🎉

ethanfrey · 2021-04-13T14:23:27Z

Bumping to beta3 in a new PR now. #269

yihuang marked this pull request as draft April 2, 2021 02:15

ethanfrey mentioned this pull request Apr 6, 2021

Thoughts on addresses CosmWasm/cosmwasm#802

Merged

yihuang force-pushed the update-cosmwas-std branch from 137f326 to 267eb7f Compare April 8, 2021 08:38

yihuang added 7 commits April 8, 2021 16:40

update cosmwasm-std, change math api

af22dfa

rename KV -> Pair

c9e8d34

address change: cw0,cw1,cw2,cw3,cw4

420dfec

address change: cw20,cw721,cw1155

470c2ba

address change: cw-controllers

e654cef

address change: multi-test

a3dde73

address change: cw20-base,cw721-base,cw1155-base

01d4e95

yihuang force-pushed the update-cosmwas-std branch from 267eb7f to 01d4e95 Compare April 8, 2021 08:42

ethanfrey reviewed Apr 8, 2021

View reviewed changes

Adjust Addr/String in cw1,3,4

be013e0

Add cw1155 package+contract to CI

57d574d

maurolacy added 5 commits April 11, 2021 13:15

Add cw1155-base cargo config

c2a2fa3

Update cw1155-base unit tests

4e105a4

Remove redundant calls

07ad611

cargo fmt

779b4fe

cargo clippy

6cb71b9

This was referenced Apr 12, 2021

Implement PrimaryKey for &Addr by removing unused parse_key #264

Closed

Take storage keys as reference (K -> &K) #265

Closed

ethanfrey and others added 5 commits April 12, 2021 15:24

Update version of wasm checker

d8894e3

Add schema to cw1155-base

e8f3eb5

Merge branch 'master' into update-cosmwas-std

81f571f

Implement PrimaryKey for &Addr

f7c0e43

Use &Addr in cw3-fixed-multisig

f35a0ab

ethanfrey marked this pull request as ready for review April 12, 2021 18:49

ethanfrey added 3 commits April 13, 2021 12:47

Use &Addr not AddrRef in cw1-subkeys

e529858

Remove AddrRef from storage-plus

91c59cc

Use &Addr not AddrRef in cw20-base

f6c8ae0

ethanfrey added 5 commits April 13, 2021 15:22

Fix cw4-stake and cw4-group

d68af14

Update cw20-staking

c0ce30e

Update cw3-flex-multisig

7583b2e

Cleanup

289d26a

Use Addr in cw1155 state keys

120857d

ethanfrey merged commit 4099566 into CosmWasm:master Apr 13, 2021

		@@ -42,7 +42,7 @@ pub struct Cw1CanonicalContract(pub CanonicalAddr);
		impl Cw1CanonicalContract {

Update cosmwasm-std #260

Update cosmwasm-std #260

Conversation

yihuang commented Apr 2, 2021

ethanfrey commented Apr 6, 2021

ethanfrey commented Apr 7, 2021

yihuang commented Apr 8, 2021

maurolacy commented Apr 8, 2021 • edited

yihuang commented Apr 8, 2021

maurolacy commented Apr 8, 2021 • edited

maurolacy commented Apr 8, 2021 • edited

ethanfrey commented Apr 8, 2021

ethanfrey commented Apr 8, 2021

yihuang commented Apr 8, 2021 • edited

ethanfrey commented Apr 8, 2021

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

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

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

Choose a reason for hiding this comment

ethanfrey commented Apr 8, 2021

yihuang commented Apr 8, 2021 • edited

ethanfrey commented Apr 8, 2021

ethanfrey commented Apr 8, 2021

maurolacy commented Apr 12, 2021

maurolacy commented Apr 12, 2021 • edited

ethanfrey commented Apr 12, 2021 • edited

yihuang commented Apr 13, 2021 • edited

maurolacy commented Apr 13, 2021

ethanfrey commented Apr 13, 2021

webmaster128 commented Apr 13, 2021

ethanfrey commented Apr 13, 2021

maurolacy commented Apr 8, 2021 •

edited

maurolacy commented Apr 8, 2021 •

edited

maurolacy commented Apr 8, 2021 •

edited

yihuang commented Apr 8, 2021 •

edited

yihuang commented Apr 8, 2021 •

edited

maurolacy commented Apr 12, 2021 •

edited

ethanfrey commented Apr 12, 2021 •

edited

yihuang commented Apr 13, 2021 •

edited