Add cw1155 specification #247

yihuang · 2021-03-17T08:06:14Z

retry of #245
fixes #246

Design decisions:

Fungible tokens and non-fungible tokens are treated equally, non-fungible tokens just have one max supply.
Transfer permission is approved or disapproved to some operator over entire set of tokens. (Scoped control is defined in ERC1761, we can port that separately)
Metadata query and token enumeration should be done by indexing attributes in block-results off-chain.
Mint and burn are mixed with transfer/send messages to keep number of message types small, otherwise we'll need much more message types like Mint/MintToContract/BatchMint/BatchMintToContract, etc.
In transfer/send messges, from/to are optional, a None from means minting, a None to means burning, they must not both be None at the same time.

Design decisions: - Fungible tokens and non-fungible tokens are treated equally, non-fungible tokens just have one max supply. - Approval is set or unset to some operator over entire set of tokens. (More nuanced control is defined in [ERC1761](https://eips.ethereum.org/EIPS/eip-1761), do we want to merge them together?) - Metadata and token enumeration should be done by indexing events off-chain. - Mint and burn are mixed with transfer/send messages, otherwise, we'll have much more message types, e.g. `Mint`/`MintToContract`/`BatchMint`/`BatchMintToContract`, etc. In transfer/send messges, `from`/`to` are optional, a `None` `from` means minting, a `None` `to` means burning, they must not both be `None` at the same time.

ethanfrey

Very nice and clean implementation and I really like some of the ideas. Especially events, but also merging eg. Send/SendFrom into one message type (avoids lots of duplication).

Most of the comments are minor suggestions to polish off the spec. The only real issue is combining send/burn/mint into one message. I would like a 2nd (or 3rd) opinion here. This is really about API design not coding.

Also, what do you think in general of extending the Ethereum spec to match our framework a bit I mean, you use Uint128 rather than Uint256 (128 bit is plenty large enough, and they just have 256 cuz everything is 256bit in Ethereum). It is a chance to take these same ideas and enhance them with some of the capabilities of CosmWasm (especially cheap iteration over parts of a set)

packages/cw0/src/event.rs

packages/cw1155/Cargo.toml

packages/cw1155/README.md

ethanfrey · 2021-03-17T21:15:33Z

packages/cw1155/README.md

+
+- Mint and burn are mixed with transfer/send messages, otherwise, we'll have much more message types, e.g. `Mint`/`MintToContract`/`BatchMint`/`BatchMintToContract`, etc.
+
+  In transfer/send messges, `from`/`to` are optional, a `None` `from` means minting, a `None` `to` means burning, they must not both be `None` at the same time.


This seems dangerous to me, as the input is JSON, and extra fields are ignored. So changing from to form turns a transfer to a mint (likely to fail anyway), but using too vs to will make you burn your tokens, rather than have the tx rejected by the chain.

@willclarktech and @maurolacy I think we discussed the issue of nullable fields changing functionality to be dangerous. Will especially in the context of CosmJS, some case when we said omitting a field from "ChangeOwner" would make it "ClearOwner" (making no one owner) could be an expensive mistake with a typo.

Can you please comment on this design here (or better look at the corresponding type definitions) and say if you approve or suggest ways to improve.

What about minting/burning if the null address is provided, like Ethereum's 0x0000000000000000000000000000000000000000? Then at least you have to specify it. Otherwise I'd suggest mint/burn boolean options to be explicit.

ethanfrey · 2021-03-17T21:17:14Z

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.
+
+`SendFrom{from, to, token_id, value, msg}` - This transfers some amount of tokens between two accounts. `to` 


The fact that you combine Send and SendFrom is nice (just provide your own address as from to make it a normal send). Just not sure to throw in Mint/MintFrom/Burn/BurnFrom here

Now, mint/burn messages are separated.

ethanfrey · 2021-03-17T21:26:51Z

packages/cw1155/src/event.rs

+}
+
+impl Event for TransferEvent {
+    fn write_attributes(&self, attributes: &mut Vec<Attribute>) {


Response object now has an add_attribute method. Maybe this could make use of it?

fn write_attributes(&self, resp: &mut Response) { resp.add_attributute("action", "transfer"); resp.add_attributute("token_id", self.token_id); // ... }

Not that it does anything different, but seems a bit cleaner to read and sets a nice pattern for other devs. (And I would like your feedback on the method name add_attribute, we are still in beta, when it can be changed)

Not very good at picking english names though ;D
add_attribute sounds good to me, I changed the method name in Event trait to add_attribututes too.

packages/cw1155/src/msg.rs

packages/cw1155/src/query.rs

ethanfrey · 2021-03-17T21:34:24Z

packages/cw1155/src/msg.rs

+        msg: Option<Binary>,
+    },
+    /// Enable or disable approval for a third party ("operator") to manage all of the caller's tokens.
+    SetApprovalForAll { operator: HumanAddr, approved: bool },


In cw721 we took a few liberties to "improve" the Ethereum design, even with the "all or nothing" approval. How about adding an optional expiration date?

Adding approvals on token by token basis would be nice as well, even if it is full control of the balance on that token_id. Are you concerned about performance? Or prefer the API this way? Especially iteration is MUCH more efficient in CosmWasm/Cosmos SDK than Ethereum and it is nice to embrace it.

I think the only trade-off is use cases vs simplicity and on-chain resource usage.
For the example cw721 contract to support approval by token, it needs to store a Vec<Approval> in TokenInfo, and need to do O(n) search when checking permission.

Fair enough. This does add more runtime complexity. We can leave it as is.

packages/cw1155/src/receiver.rs

yihuang · 2021-03-18T07:57:11Z

Very nice and clean implementation and I really like some of the ideas. Especially events, but also merging eg. Send/SendFrom into one message type (avoids lots of duplication).

Most of the comments are minor suggestions to polish off the spec. The only real issue is combining send/burn/mint into one message. I would like a 2nd (or 3rd) opinion here. This is really about API design not coding.

Also, what do you think in general of extending the Ethereum spec to match our framework a bit I mean, you use Uint128 rather than Uint256 (128 bit is plenty large enough, and they just have 256 cuz everything is 256bit in Ethereum). It is a chance to take these same ideas and enhance them with some of the capabilities of CosmWasm (especially cheap iteration over parts of a set)

Thanks for review, new modifications:

Add metadata and token enumeration query extension similar to cw721
Modified ApprovalForAll similar to cw721
Add some comments

Open issues:

Should we mix mint/burn messages with transfer/send? comments
Should we support approval by token, like cw721? comments

ethanfrey · 2021-03-18T08:16:05Z

Thanks for the updates, looking very nice.

Should we support approval by token, like cw721?

I don't know if this use case will be used much or at all and it does indeed add runtime overhead to every check. I am fine to leave it off.

Should we mix mint/burn messages with transfer/send?

This is my only outstanding concern, and I would like the comment of another developer with CosmWasm experience here. Otherwise, it looks good.

ethanfrey

Looks good minus the discussion on separating Transfer/Mint/Burn.

If you wish to reduce types, another thought would be to combine TransferFrom and SendFrom.

eg.

/// This handles contract call or user account
SendFrom {
  from: HumanAddr, // either env.sender self or another account sender is approved on
  to: HumanAddr, // an external account or contract 
  token_id: TokenId,
  value: Uint128,
  // if msg is None, just transfer the tokens.
  // if msg is Some(m), then call a Cw1155 receiver on a contract with the `to` address (after moving the token)
  msg: Option<Binary>,
}

And then add other types for minting and burning (or at least burning).
This would allow you to reduce the number of types and address my concern about possibly mixing burning and send.

If you want to still keep from: None to mean mint I have less issues than with burn. In the end maybe you can end up with the followng types:

SendFrom - mints or transfers from my account or transfers from other account. If there is a message, it will assume the target is a contract and invoke the cw1155 receiver on it.
BatchSendFrom - like SendFrom but with a list of tokens
BurnFrom - burns one token either from my account or an account i was authorized on
ApproveAll
RevokeAll

ethanfrey · 2021-03-18T08:38:27Z

packages/cw1155/README.md


 `SendFrom{from, to, token_id, value, msg}` - This transfers some amount of tokens between two accounts. `to` 
 must be an address controlled by a smart contract, which implements
-the `CW1155Receiver` interface. The operator should have approval from the source account. The `msg` will be passed to the recipient contract, along with the other fields.
+the `CW1155Receiver` interface. The operator should eitherbe the `from` account or have approval from it. The `msg` will be passed to the recipient contract, along with the other fields.


either be (missing space)

ethanfrey · 2021-03-18T08:40:14Z

packages/cw1155/README.md

+`ApprovedForAll{owner, include_expired, start_after, limit}` - List all operators that can
+access all of the owner's tokens. Return type is `ApprovedForAllResponse`.
+If `include_expired` is set, show expired owners in the results, otherwise,
+ignore them. If query for some specific operator, set `start_after` to the operator, and set limit to 1.


Actually interesting point - start_after does not include that value itself. It is great for pagination, but not good for finding one in particular. Maybe you do want the other query as well to just get one.

added ApprovedForAllItem (please feel free to suggest a name for this ;).

yihuang · 2021-03-18T10:30:45Z

  // if msg is None, just transfer the tokens.
 // if msg is Some(m), then call a Cw1155 receiver on a contract with the `to` address (after moving the token)
 msg: Option<Binary>,

I think it needs to be Option<Option<Binary>>:

None, normal recipient address
Some(None), recipient address is contract, but send None as extra msg.
Some(Some(msg)), recipient address is contract, send extra msg.

BurnFrom - burns one token either from my account or an account i was authorized on

I guess we probably also want BatchBurnFrom?

ethanfrey · 2021-03-18T13:57:07Z

I think it needs to be Option<Option>:

Option<Option<Binary>> and Option<Binary> have the same JSON representation.

There is no real use case when we send a message to a contract with data None. You can always send Some(b"{}") for the cases when it doesn't matter (and ensure you have a valid JSON object as body)

packages/cw1155/src/query.rs

yihuang · 2021-03-19T02:04:35Z

I think it needs to be Option:

Option<Option<Binary>> and Option<Binary> have the same JSON representation.

There is no real use case when we send a message to a contract with data None. You can always send Some(b"{}") for the cases when it doesn't matter (and ensure you have a valid JSON object as body)

But the receiver msg contains Option<Binary>, do you think we should remove the Option here too?

ethanfrey · 2021-03-19T13:31:59Z

But the receiver msg contains Option, do you think we should remove the Option here too?

You are right. I designed the cw20 receiver interface before implementing any contracts. It would be good to remove Option there.

yihuang · 2021-03-19T17:53:54Z

Merged TransferFrom into SendFrom
Separated out Mint/Burn messages

- Seperate Mint/Burn/BatchMint/BatchBurn - Merge TransferFrom and SendFrom

ethanfrey

Thank you for the changes

ThienLK1 · 2022-03-07T09:33:53Z

packages/cw1155/README.md

@@ -54,10 +54,10 @@ Any contract wish to receive CW1155 tokens must implement `Cw1155ReceiveMsg` and

  `from`/`to` are optional, no `from` attribute means minting, no `to` attribute means burning.

- `metadata(url, token_id)`
+- `token_info(url, token_id)`


In event.rs i only see MetadataEvent, not found token_info.
And i dont see any api using this event.
I just want add uri for token, but dont find any api do this?

metadata modification is not part of the standard API, different contracts can implement their own APIs to do that I think.

Do you mean base on ERC1155Metadata?
Do you think it can fulfill what's missing?

yihuang force-pushed the cw1155-spec branch from 6c71fb7 to f721945 Compare March 17, 2021 08:21

update schema

3fd5e70

ethanfrey reviewed Mar 17, 2021

View reviewed changes

yihuang added 4 commits March 18, 2021 14:47

add TokenInfo query api

d9d54d3

Add Enumerable query apis similar to CW721

7354e36

Change approval apis similar to CW721

242a9f6

Add comments to Cw1155ReceiveMsg

e750e93

update schema

3bc9208

ethanfrey reviewed Mar 18, 2021

View reviewed changes

Add ApprovedForAllItem to query approved status for one operator

c6d0ab5

ethanfrey reviewed Mar 18, 2021

View reviewed changes

packages/cw1155/src/query.rs Outdated Show resolved Hide resolved

yihuang added 2 commits March 19, 2021 10:09

rename query nanme ApprovedForAllItem to IsApprovedForAll

0283499

format readme, breaking long lines

16d0158

Restructure message types

8581364

- Seperate Mint/Burn/BatchMint/BatchBurn - Merge TransferFrom and SendFrom

yihuang force-pushed the cw1155-spec branch from 401063d to 8581364 Compare March 19, 2021 17:57

ethanfrey approved these changes Mar 19, 2021

View reviewed changes

ethanfrey merged commit 34504b0 into CosmWasm:master Mar 19, 2021

ThienLK1 reviewed Mar 7, 2022

View reviewed changes

yihuang deleted the cw1155-spec branch March 7, 2022 09:39

webmaster128 mentioned this pull request Nov 10, 2023

Deprecate and remove cw_utils::Event CosmWasm/cw-minus#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cw1155 specification #247

Add cw1155 specification #247

yihuang commented Mar 17, 2021 •

edited

Loading

ethanfrey left a comment

ethanfrey Mar 17, 2021

willclarktech Mar 18, 2021

ethanfrey Mar 17, 2021

yihuang Mar 19, 2021

ethanfrey Mar 17, 2021

yihuang Mar 18, 2021 •

edited

Loading

ethanfrey Mar 17, 2021

yihuang Mar 18, 2021 •

edited

Loading

ethanfrey Mar 18, 2021

yihuang commented Mar 18, 2021 •

edited

Loading

ethanfrey commented Mar 18, 2021

ethanfrey left a comment

ethanfrey Mar 18, 2021

yihuang Mar 19, 2021

ethanfrey Mar 18, 2021

yihuang Mar 18, 2021 •

edited

Loading

yihuang commented Mar 18, 2021

ethanfrey commented Mar 18, 2021

yihuang commented Mar 19, 2021

ethanfrey commented Mar 19, 2021

yihuang commented Mar 19, 2021

ethanfrey left a comment

ThienLK1 Mar 7, 2022

yihuang Mar 7, 2022

ThienLK1 Mar 7, 2022


		- Mint and burn are mixed with transfer/send messages, otherwise, we'll have much more message types, e.g. `Mint`/`MintToContract`/`BatchMint`/`BatchMintToContract`, etc.

		In transfer/send messges, `from`/`to` are optional, a `None` `from` means minting, a `None` `to` means burning, they must not both be `None` at the same time.


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

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

Add cw1155 specification #247

Add cw1155 specification #247

Conversation

yihuang commented Mar 17, 2021 • edited Loading

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

yihuang Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang commented Mar 18, 2021 • edited Loading

ethanfrey commented Mar 18, 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

Choose a reason for hiding this comment

yihuang Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

yihuang commented Mar 18, 2021

ethanfrey commented Mar 18, 2021

yihuang commented Mar 19, 2021

ethanfrey commented Mar 19, 2021

yihuang commented Mar 19, 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

Choose a reason for hiding this comment

yihuang commented Mar 17, 2021 •

edited

Loading

yihuang Mar 18, 2021 •

edited

Loading

yihuang Mar 18, 2021 •

edited

Loading

yihuang commented Mar 18, 2021 •

edited

Loading

yihuang Mar 18, 2021 •

edited

Loading