implement demo cw1155 contract #251

yihuang · 2021-03-23T07:52:31Z

there are several more unit tests to be done, the overall implementation is good for review now.
There are two FIXME currently:

code, we should add overflow error branch in StdError.
code, the list returned by query is out of order, I'm not sure if it's an issue of the test storage itself?

ethanfrey

I just got back from vacation and have lots to do before I can take a deeper look, but I did add comments on the 2 FIXMEs. These are subtle points about how cosmwasm-std is designed and actually intended.

ethanfrey · 2021-03-29T08:49:03Z

contracts/cw1155-base/src/contract.rs

+    }
+}
+
+fn checked_add(a: Uint128, b: Uint128) -> StdResult<Uint128> {


Interesting. We define sub for Uint128 to possibly return an error if it underflows (not enough balance). We don't do this for adding as we assumed this would never happen.

We do however, test that this extremely rare condition causes a panic, which is enough to keep the contract secure. It won't provide a pretty error message to the user, but will block the attack vector. I would just use the normal Uint128 + Uint128 approach

I reverted back to simple + operator.
Opened issue in cosmwasm-std for the overflow handling: CosmWasm/cosmwasm#850

ethanfrey · 2021-03-29T08:52:14Z

contracts/cw1155-base/src/contract.rs

+            .unwrap();
+        }
+
+        // // FIXME test failed, query result out of order


They are not "out of order", it is that the keys are stored in CanonicalAddr, which you cannot assume is the same ordering as HumanAddr. For bech32 the order sometimes is the same, sometimes different. For tests, we semi-randomize the mapping to ensure you do not depend on it.

If you hardcode some addresses, just figure out via a few test runs what the ordering is as CanonicalAddr and then assert that one. It will be stable, but it makes you be sure if you are comparing canonical or human addresses.

Discussion here: CosmWasm/cosmwasm#552

thanks for clarification, FIXME is resolved now.

yihuang · 2021-03-29T10:04:35Z

I just got back from vacation and have lots to do before I can take a deeper look, but I did add comments on the 2 FIXMEs. These are subtle points about how cosmwasm-std is designed and actually intended.

Thanks, two FIXMEs are resolved now.

yihuang · 2021-04-01T16:13:18Z

Added unit tests to expect panic when minting token amount overflow.

ethanfrey

Very nice work. And sorry for the delay in reviewing, between vacation and Easter and end-of-the-month, my work weeks have been full.

I made some stylistic comments on a few places, but quite impressed with the quality of the code. And I learned a few techniques I'd like to emulate for other contracts. Especially the event system and inlining those query functions.

If you want to take a pass and address some of the minor comments, I am happy to merge it. Very nice addition to cosmwasm-plus

contracts/cw1155-base/Cargo.toml

contracts/cw1155-base/src/lib.rs

contracts/cw1155-base/src/state.rs

packages/cw1155/src/event.rs

contracts/cw1155-base/src/contract.rs

ethanfrey · 2021-04-06T20:42:19Z

contracts/cw1155-base/src/contract.rs

+            .unwrap(),
+            Response {
+                attributes: vec![
+                    attr("action", "transfer"),


This is interesting. I am not sure how this ends up when it comes out through tendermint (I think it does some "collapsing" of events).

It totally makes sense. I wonder if you had a wish on a cleaner way to encode these or pass them through to the client? Although I guess just as it is is perfect for searching.

I'm not sure how it's going to be indexed by tendermint though, need to look into details in tendermint I guess.

EDIT:
Yeah, i guess it would be fine, since tendermint will index attributes independently?

Yeah, they should all match

test approval expires

- Use entry_point macro - bump dependencies to alpha3 - Remove unused struct - Remove guard helper - Other misc changes

ethanfrey · 2021-04-08T08:03:17Z

contracts/cw1155-base/src/contract.rs

-    rsp.messages = if let Some(msg) = msg {
-        vec![Cw1155ReceiveMsg {
+    if let Some(msg) = msg {
+        rsp.messages = vec![Cw1155ReceiveMsg {


This could also be rsp.add_message(Cw1155ReceiveMsg { .. }.into_cosmos_msg(to)?);

But I don't think that is any clearer in this case.

ethanfrey · 2021-04-08T08:03:32Z

contracts/cw1155-base/src/contract.rs

        // insert if not exist
        let key = TOKENS.key(&token_id);
        if deps.storage.get(&key).is_none() {
+            // insert an empty entry so token enumeration can find it


thanks for the comment

ethanfrey · 2021-04-08T08:03:44Z

contracts/cw1155-base/src/contract.rs

@@ -42,6 +44,7 @@ pub struct ExecuteEnv<'a> {
    info: MessageInfo,
 }

+#[cfg_attr(not(feature = "library"), entry_point)]


Nice update

ethanfrey · 2021-04-08T08:04:26Z

contracts/cw1155-base/src/contract.rs

-    if !check_can_approve(deps.as_ref(), &env, &from, &info.sender)? {
-        return Err(ContractError::Unauthorized {});
-    }
+    guard_can_approve(deps.as_ref(), &env, &from, &info.sender)?;


Yeah, this is a nicer way to inline the common check.

ethanfrey · 2021-04-08T08:06:06Z

contracts/cw1155-base/src/contract.rs

+            .unwrap(),
+            Response {
+                attributes: vec![
+                    attr("action", "transfer"),


Yeah, they should all match

ethanfrey

Good stuff.

ethanfrey · 2021-04-08T08:11:16Z

Created #261 inspired by your comments

yihuang force-pushed the cw1155-impl branch from fe56d46 to dc99162 Compare March 24, 2021 07:57

ethanfrey reviewed Mar 29, 2021

View reviewed changes

yihuang force-pushed the cw1155-impl branch from dc99162 to 76d783b Compare March 29, 2021 10:01

yihuang force-pushed the cw1155-impl branch 2 times, most recently from a1b7cb0 to 72ea5e8 Compare April 1, 2021 16:08

yihuang requested a review from ethanfrey April 1, 2021 16:13

ethanfrey approved these changes Apr 6, 2021

View reviewed changes

implement demo cw1155 contract

829ace7

test approval expires

yihuang force-pushed the cw1155-impl branch from 72ea5e8 to 60640f7 Compare April 8, 2021 03:38

fix pull request suggestions

6f65c66

- Use entry_point macro - bump dependencies to alpha3 - Remove unused struct - Remove guard helper - Other misc changes

yihuang force-pushed the cw1155-impl branch from 60640f7 to 6f65c66 Compare April 8, 2021 03:54

encapsulate guard_can_approve

824fc41

ethanfrey reviewed Apr 8, 2021

View reviewed changes

ethanfrey approved these changes Apr 8, 2021

View reviewed changes

ethanfrey mentioned this pull request Apr 8, 2021

Add existence helpers to cw-storage-plus #261

Closed

ethanfrey merged commit 7244314 into CosmWasm:master Apr 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement demo cw1155 contract #251

implement demo cw1155 contract #251

yihuang commented Mar 23, 2021 •

edited

ethanfrey left a comment

ethanfrey Mar 29, 2021

yihuang Mar 29, 2021

ethanfrey Mar 29, 2021

yihuang Mar 29, 2021

yihuang commented Mar 29, 2021

yihuang commented Apr 1, 2021 •

edited

ethanfrey left a comment

ethanfrey Apr 6, 2021

yihuang Apr 8, 2021 •

edited

ethanfrey Apr 8, 2021

ethanfrey Apr 8, 2021

ethanfrey Apr 8, 2021

ethanfrey Apr 8, 2021

ethanfrey Apr 8, 2021

ethanfrey Apr 8, 2021

ethanfrey left a comment

ethanfrey commented Apr 8, 2021

implement demo cw1155 contract #251

implement demo cw1155 contract #251

Conversation

yihuang commented Mar 23, 2021 • edited

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

yihuang commented Mar 29, 2021

yihuang commented Apr 1, 2021 • edited

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Apr 8, 2021 • edited

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 left a comment

Choose a reason for hiding this comment

ethanfrey commented Apr 8, 2021

yihuang commented Mar 23, 2021 •

edited

yihuang commented Apr 1, 2021 •

edited

yihuang Apr 8, 2021 •

edited