Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rfc(0040): add sudt info #290

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

xcshuan
Copy link

@xcshuan xcshuan commented Dec 25, 2021

No description provided.

@xcshuan xcshuan requested a review from a team as a code owner December 25, 2021 14:03
@xcshuan xcshuan requested review from doitian and knwang December 25, 2021 14:03
extra: {website:"https://nervos.org"} // optional
```

The serialization format of sudt info should be json.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 drawbacks to use json:

  1. waste of space
  2. the smart contracts will need a json library, which will introduce security issue.

I think molecule is OK.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if Molecule can be used here as it's more space-efficient and a default on CKB. If I remember correctly, the reason for using JSON than Molecule was the ease of use for application developers? Not sure if Molecule's javascript support renders the reason obsolete?

Copy link
Member

@janx janx Jan 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xcshuan any thoughts on this one?

cc @CipherWang @louzhixian @Keith-CY

Copy link
Author

@xcshuan xcshuan Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use the simplest format, which is repeated length+content.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple format is consistent with sUDT indeed. But sUDT info data consists of several key/value pairs rather than a single number in sUDT data. Use length+content format here basically reinvents Molecule, with the drawback of lacking deserialization tools.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a library for molecule available, I think it should be better to use molecule.

There are libraries for C, Rust, go and JavaScript: https://github.com/nervosnetwork/molecule.

At least we need to verify in the script whether the data of the info cell is a valid SUDT Info. I don't know if using json will make this troublesome.

It will. Use JSON in on-chain contract is hard, expensive and may bring security risks as Jindong mentions. Do we really need to verify sudt info format in scripts? It doesn't make sense in my opinion.

If we skip the verification, we can stick to the original method.

If we do need that, I recommend use the format below:

vector Bytes <byte>;

table SudtInfo {
    decimals: byte,
    name: Bytes,
    symbol: Bytes,
    extra: Bytes,
}

The extra may be a problem in real world cases. Since molecule needs schema to parse data, but different user may use different fields here. The best solution for this field I can find is to use an embedded JSON string. In this case, on-chain contracts will only verify the fixed fields with molecule, and we have to develop some tools for users in the SDK to parse the whole struct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer not to verify format in CKB script. It simplifies the script/standard a lot. If someone creates an ill-formated SUDT info cell intentionally or accidentally, applications can simply ignore the bad info cell and the result is the same as no SUDT info cell.

Copy link
Member

@janx janx Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary: we're considering JSON vs. Molecule, while JSON is simple and widely used, its drawback is more difficult to parse/validate on chain. However it seems unnecessary to include SUDT info parse/validation in on-chain script, which basically avoids the problem.

In this case I suggest to go with JSON and skip on-chain format validation, to keep things simple.

Copy link
Member

@janx janx Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xcshuan I suggest adding @huwenchao as 2nd author because it's his analysis and suggestions and having him complete the RFC, if you agree.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding @huwenchao as 2nd author

Ok, I think it's fine.

rfcs/0000-sudt-info/0000-sudt info.md Outdated Show resolved Hide resolved
rfcs/0000-sudt-info/0000-sudt info.md Outdated Show resolved Hide resolved
rfcs/0000-sudt-info/0000-sudt info.md Outdated Show resolved Hide resolved
rfcs/0000-sudt-info/0000-sudt info.md Outdated Show resolved Hide resolved
@janx
Copy link
Member

janx commented Jan 10, 2022

@xcshuan thoughts on above feedbacks?

XuJiandong
XuJiandong previously approved these changes Jan 12, 2022
Copy link
Contributor

@XuJiandong XuJiandong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've no further question for this PR.

rfcs/0000-sudt-info/0000-sudt info.md Outdated Show resolved Hide resolved
rfcs/0000-sudt-info/0000-sudt info.md Outdated Show resolved Hide resolved
rfcs/0000-sudt-info/0000-sudt info.md Outdated Show resolved Hide resolved
- **Rule 1:** validate the format of info cell data.
- **Rule 2:** The args conform to the rules of type id.
- **Rule 3:** The lockscript of info_cell should be set to deadlock by default, in some cases, developers can choose other lockscript.
- **Rule 4:** After constructing this Info cell, use the hash of info_cell's type_script as the first 32 bytes of the args of the lockscript of SUDT owner.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this rule, there is no way to add sudt info for existing script-drive SUDT with this protocol, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, info is bound to the token.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a problem. Since token from NexisDAO and Force Bridge can not use this protocol then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consulted Nexisdao, they have designed this, and udt info typescript can't actually sense whether a udt is controlled by a private key or not, it should first judge according to the script-driven mode, and then judge according to the single-owner mode , so for the info cell of udt originating from forcebridge, it can be generated according to the mode of single owner, as long as it is the earliest one.

@TheWaWaR
Copy link
Contributor

TheWaWaR commented Jan 23, 2022

May better rename 0000-sudt info.md to 0000-sudt-info.md (replace " " with "-") for consistency?

@doitian doitian mentioned this pull request Apr 1, 2022
14 tasks
Copy link
Member

@doitian doitian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reserved RFC Number 0040 for this PR. See details in #246

@doitian doitian changed the title add sudt info rfc(0040): add sudt info Apr 1, 2022
doc: remove the sudt info verification
@phroi
Copy link
Contributor

phroi commented Oct 25, 2022

Hi!! I just found out about this proposal from the discord Github bot! I'm the guy working on iCKB, so basically I'm the new guy in Nervos, still let me add my opinion.

I don't believe adding this information on-chain is the right step, reason being no on-chain logic needs this metadata. Then again there is a very good use-case for this metadata in DApps, so makes sense to have it somewhere in an easily accessible format.

So why not standardize a schema (fields names, types, which are required and which are optional), a format (JSON?), a location (IPFS?) and define them off-chain?

Some time ago I was reasoning about this very same problem with Frank@Lay2, I suggested using IPFS to store this information. Right now I see there are more alternatives:

  1. If every token keeps the metadata in a common file, then this file needs to be updated for every new token, so there are two ways to go about it:
    a. Anyone can create a new revision of this file with the proposed changes and broadcast the new IPFS link in a common medium: a dedicated discord thread and/or a new issue per new token in a github repository and/or smoke signals? Whatever works!! Others may accept or not the changes independently.
    b. Alternatively, only a specific subgroup can update this common knowledge, then IPFS is unneeded, just a GitHub repository is necessary. Still this is a quite centralized approach.

  2. If every token keeps the metadata in a different file, then IPFS is unneeded, this information can reside in the repositories containing the token contracts, possibly in a standardized location (for ex. /metadata.json). Each DApp developer will then integrate this metadata for every token the DApp supports.

Another benefit of adopting an off-chain approach is the compatibility with older SUDT tokens, which is actually a nice side effect!!

Feel free to let me know your thoughts on this,
Phroi

@CipherWang
Copy link
Contributor

Agreed with @phroi. It's not necessary to turn to a live cell to store such static information since no other scripts would access that. We can put these data to tx.witnesses in a pre-defined data schema. IPFS is great, but the accessibility is not good enough.

@phroi
Copy link
Contributor

phroi commented Oct 26, 2022

Yeah!! IPFS data (without using gateways) is difficult to access, but I pointed at it because it's addressed by content, so simplifies the process.. still without using IPFS, one could store this metadata anywhere and by just adding a signature, we can make sure it has not been tampered with.

So I see this flow for DApp developers:

  • They develop a DApp and add the metadata relative to the supported tokens.
  • A few months later they add support for new tokens and so they update the metadata
  • ...

So basically, the way I see it, the metadata is always statically added to website, not dynamically loaded. This approach avoids Metadata XSS Attacks altogether, because the developer will carefully check the new data!

@CipherWang what competitive advantage do you see for having this data on-chain?

@CipherWang
Copy link
Contributor

@CipherWang what competitive advantage do you see for having this data on-chain?

If the basic data is on-chain, like what Ethereum does, everyone who devs on CKB can immediately access the data. We has proposed a metadata schema stored in the witnesses field. You can check it out if interested. https://www.cotadev.io/docs/protocols/CTMeta

@phroi
Copy link
Contributor

phroi commented Oct 29, 2022

Hi @CipherWang, thanks for taking your time to reply, very appreciated!!

If the basic data is on-chain, like what Ethereum does

Yeah, about Ethereum, for example ERC721 have (almost) all of its metadata off-chain: https://docs.opensea.io/docs/metadata-standards#implementing-token-uri

Switching back to our case, having metadata off-chain would simplify devs experience by allowing token devs to include more heavy metadata like the token's icon, which is basically a requirement when supporting a token in a DApp.

Metadata schema stored in the witnesses field https://www.cotadev.io/docs/protocols/CTMeta

Ten days ago @janx pointed me to the COTA standard and since it uses Merkle trees of course I love it (still it's to be seen how it will affect developer experience). Switching back to CTMeta, I see the token Metadata is a work in progress, still from the CTMeta convention section I got the gist of it (BTW I made a pull request to fix some formatting issues).

Basically since the witness only pays transaction fees, then it's cheaper, so one can carry-around the associated cell metadata in the witness. Yeah, it's a smart idea, still I don't know if replicating the metadata information for each transaction does really make sense. For example following CTMeta approach, since it doesn't really make sense to fit all the metadata (like the token icon) into the witness, then it will always happen that either:

  • some metadata is stored in a global on-chain location (like L1 scripts) and the witness contains a reference to it.
  • some metadata is stored in a off-chain location (like Ethereum NFTs and metadata) and the witness contains its URI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants