port erc-1155 to cosmwasm #245

yihuang · 2021-03-15T02:29:17Z

https://github.com/CosmWasm/cosmwasm-plus/blob/8adba30729f0c8da16bdb1c834991380d111d365/packages/cw1155/README.md

Open issues:

Should contract store metadata and token set (for enumeration) on chain, or it should be done by off-chain indexing service?

CLAassistant · 2021-03-15T02:29:21Z

All committers have signed the CLA.

orkunkl · 2021-03-16T06:42:39Z

Thank you I will review when I get the chance.

tomtau

👍

tomtau · 2021-03-16T07:04:47Z

contracts/cw1155-base/src/contract.rs

+            token_id: token_id.clone(),
+            msg,
+        }
+        .into_cosmos_msg(to.clone().unwrap())?], // `to` in Send messages must not be None


not sure if MIRAI contracts can be used to express this?

packages/cw1155/src/lib.rs

tomtau · 2021-03-16T07:31:55Z

packages/cw1155/README.md

+
+## Metadata and Enumerable
+
+[TODO] ERC1155 suggests that metadata and enumerable should be indexed from events log, to save some on-chain storage. Should we define standard events like ERC1155?


probably ok, but not sure if in line with the rest of cosmwasm-plus, as a kind of the goal seemed to have been to allow direct querying in cosmjs https://github.com/cosmos/cosmjs/tree/main/packages/cosmwasm-stargate/src/queries etc.

We do emit events (well attributes) on most actions.
It would be good to properly define them to allow easy querying.

If you want to define the Events in the cw1155 spec, then we would be happy to review. It would set a good example and we could update the other modules to follow it.

ethanfrey

First pass, just looking at the spec.

Thanks for making this PR.

ethanfrey · 2021-03-16T07:40:21Z

packages/cw1155/README.md

+
+## Metadata and Enumerable
+
+[TODO] ERC1155 suggests that metadata and enumerable should be indexed from events log, to save some on-chain storage. Should we define standard events like ERC1155?


We do emit events (well attributes) on most actions.
It would be good to properly define them to allow easy querying.

If you want to define the Events in the cw1155 spec, then we would be happy to review. It would set a good example and we could update the other modules to follow it.

packages/cw1155/src/lib.rs

ethanfrey · 2021-03-16T07:43:27Z

packages/cw1155/src/msg.rs

+#[derive(Serialize, Deserialize, Clone, PartialEq, JsonSchema, Debug)]
+#[serde(rename_all = "snake_case")]
+pub enum Cw1155HandleMsg {
+    /// TransferFrom is a base message to move tokens


Interesting, there is no specific Transfer message, but if from is None, then we send from the info.sender account? What happens if the to field is None? I assume that is required, like SendFrom.contract

I'm a bit surprised to see Options here, can you just add a line of comment explaining what they do

Both from and to are Option, if from is None, it means mint new tokens, if to is None, it means burn tokens. They must not both be None at the same time.

In the spec there's a line to explain, Option is used to mean minting(from is None) and burning(to is None).

ethanfrey · 2021-03-16T07:44:44Z

packages/cw1155/src/receiver.rs

+#[serde(rename_all = "snake_case")]
+pub struct Cw1155ReceiveMsg {
+    pub operator: HumanAddr,
+    pub from: Option<HumanAddr>,


From should always be set, right? Otherwise you don't know who is calling this.

What is the different between from and operator?

When from is None, it means new tokens are minted directly to the receiver by operator.

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>

ethanfrey

A few more comments.

I think it would be best to break this into 2 PRs.
Can you make one for the packages/cw1155 first? Then we can discuss the design issues?

It doesn't make sense to review the code until the interface is clear. This way we could merge the package/spec quickly and then you work on updating the implementation to match in a separate PR

ethanfrey · 2021-03-16T09:12:08Z

packages/cw1155/README.md

+
+`TransferFrom{from, to, token_id, value}` - This transfers some amount of tokens between two accounts. The operator should have approval from the source account.
+
+Both `from` and `to` are `Option`, if `from` is `None`, it means mint new tokens, if `to` is `None`, it means burn tokens. They must not both be `None` at the same time.


I now see the description here (I asked for it below). It would be good to have such essential docs also in the rustdoc on the types.

Having "transfer my tokens", "transfer your tokens", "mint new tokens", "burn my tokens" and "burn your tokens" all in the same message type does reduce some boilerplate, but would make it very easy to make mistakes. Especially since JSON ignores extra fields. (And the client sneds JSON)

If they accidentally use rcpt or too instead of to, this will burn the tokens. 😱

I see the idea of joining "send mine" and "send yours" as one type, but I would keep Mint and Burn as separate message types, so it is clear what is needed - and very unlikley to accidentally type "burn_from" instead of "transfer_from"

ethanfrey · 2021-03-16T09:13:32Z

packages/cw1155/README.md

+
+Both `from` and `to` are `Option`, if `from` is `None`, it means mint new tokens, if `to` is `None`, it means burn tokens. They must not both be `None` at the same time.
+
+`SendFrom{from, to, token_id, value, msg}` - This transfers some amount of tokens between two accounts. `to` 


Two questions:

There is no mention of allowances - for SendFrom to work from other accounts, we need those, right?

This seems to support many different cw20-style tokens in one contract. Where is NFT support done?

yihuang · 2021-03-17T02:13:57Z

Thanks for reviewing, I'll make another PR soon:

only specification
more separated message branches for different cases, mint/burn/transfer/send, etc.
data types for events(attributes).

Closing this one now.

ethanfrey · 2021-03-17T08:18:54Z

Thanks for the PRs. I will review the spec shortly.

yihuang · 2021-03-17T08:27:46Z

Thanks for the PRs. I will review the spec shortly.

Still chose to mix burn/mint with transfer messages, because otherwise, there'll need much more message types to be complete.

yihuang force-pushed the cw1155 branch from 9b61197 to a3491c5 Compare March 15, 2021 02:31

yihuang added 2 commits March 15, 2021 23:21

port erc-1155 to cosmwasm

d4218c2

add unit test

8adba30

yihuang force-pushed the cw1155 branch from a3491c5 to 8adba30 Compare March 15, 2021 15:21

yihuang added 2 commits March 15, 2021 23:33

fix lint suggestions

c94dff4

more unit tests

715d47e

tomtau reviewed Mar 16, 2021

View reviewed changes

ethanfrey reviewed Mar 16, 2021

View reviewed changes

Update packages/cw1155/src/lib.rs

273e5ce

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>

ethanfrey reviewed Mar 16, 2021

View reviewed changes

yihuang closed this Mar 17, 2021

yihuang mentioned this pull request Mar 17, 2021

Add cw1155 specification #247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port erc-1155 to cosmwasm #245

port erc-1155 to cosmwasm #245

yihuang commented Mar 15, 2021 •

edited

Loading

CLAassistant commented Mar 15, 2021 •

edited

Loading

orkunkl commented Mar 16, 2021

tomtau left a comment

tomtau Mar 16, 2021

tomtau Mar 16, 2021

ethanfrey Mar 16, 2021

ethanfrey left a comment

ethanfrey Mar 16, 2021

ethanfrey Mar 16, 2021

yihuang Mar 16, 2021

ethanfrey Mar 16, 2021

yihuang Mar 16, 2021 •

edited

Loading

ethanfrey left a comment

ethanfrey Mar 16, 2021

ethanfrey Mar 16, 2021

yihuang commented Mar 17, 2021 •

edited

Loading

ethanfrey commented Mar 17, 2021

yihuang commented Mar 17, 2021 •

edited

Loading


		## Metadata and Enumerable

		[TODO] ERC1155 suggests that metadata and enumerable should be indexed from events log, to save some on-chain storage. Should we define standard events like ERC1155?


		`TransferFrom{from, to, token_id, value}` - This transfers some amount of tokens between two accounts. The operator should have approval from the source account.

		Both `from` and `to` are `Option`, if `from` is `None`, it means mint new tokens, if `to` is `None`, it means burn tokens. They must not both be `None` at the same time.


		Both `from` and `to` are `Option`, if `from` is `None`, it means mint new tokens, if `to` is `None`, it means burn tokens. They must not both be `None` at the same time.

		`SendFrom{from, to, token_id, value, msg}` - This transfers some amount of tokens between two accounts. `to`

port erc-1155 to cosmwasm #245

port erc-1155 to cosmwasm #245

Conversation

yihuang commented Mar 15, 2021 • edited Loading

CLAassistant commented Mar 15, 2021 • edited Loading

orkunkl commented Mar 16, 2021

tomtau 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

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 Mar 16, 2021 • edited Loading

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

yihuang commented Mar 17, 2021 • edited Loading

ethanfrey commented Mar 17, 2021

yihuang commented Mar 17, 2021 • edited Loading

yihuang commented Mar 15, 2021 •

edited

Loading

CLAassistant commented Mar 15, 2021 •

edited

Loading

yihuang Mar 16, 2021 •

edited

Loading

yihuang commented Mar 17, 2021 •

edited

Loading

yihuang commented Mar 17, 2021 •

edited

Loading