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

[0052] Extensible UDT #428

Merged
merged 22 commits into from
Nov 12, 2024
Merged

[0052] Extensible UDT #428

merged 22 commits into from
Nov 12, 2024

Conversation

XuJiandong
Copy link
Contributor

Extensible UDT(xUDT) is an extension of Simple UDT for defining more behaviors a UDT might need. While simple UDT provides a minimal core for issuing UDTs on Nervos CKB, extensible UDT builds on top of simple UDT for more potential needs, such as regulations.

@XuJiandong XuJiandong requested a review from a team as a code owner January 9, 2024 07:53
@doitian doitian mentioned this pull request Jan 10, 2024
14 tasks
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
XuJiandong and others added 5 commits January 19, 2024 16:16
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
XuJiandong and others added 7 commits January 23, 2024 09:58
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Yukang <moorekang@gmail.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
Co-authored-by: Flouse <1297478+Flouse@users.noreply.github.com>
doitian
doitian previously approved these changes Mar 18, 2024
janx
janx previously approved these changes Mar 23, 2024
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
rfcs/0052-extensible-udt/0052-extensible-udt.md Outdated Show resolved Hide resolved
* rename XudtWitnessInput to XudtWitness
* raw_extension_data and extension_data are not correlated. The extension_data has a new section
* behavior of multiple instances of the same extension script
* some wording
@phroi
Copy link
Contributor

phroi commented Apr 15, 2024

Hey Cryptape, iCKB here 👋 I was updating iCKB implementation from sUDT to xUDT and I stumbled upon an apparent inconsistency in the RFC:

  • On mainnet xUDT is deployed and referenced by data1 with a zero lock.
  • On testnet xUDT is deployed and referenced by type.

So my questions are:

  1. Why keeping on testnet the reference by type when the implementation on mainnet cannot be updated? 🤔
  2. Would it be possible to update the proposed RFC to reflect the reference by data1 for both? If not, why?

Keep up the Great Work,
Phroi

Use `data1` instead of `type`
@XuJiandong
Copy link
Contributor Author

Hey Cryptape, iCKB here 👋 I was updating iCKB implementation from sUDT to xUDT and I stumbled upon an apparent inconsistency in the RFC:

  • On mainnet xUDT is deployed and referenced by data1 with a zero lock.
  • On testnet xUDT is deployed and referenced by type.

So my questions are:

  1. Why keeping on testnet the reference by type when the implementation on mainnet cannot be updated? 🤔
  2. Would it be possible to update the proposed RFC to reflect the reference by data1 for both? If not, why?

Keep up the Great Work, Phroi

Done in 4c6c762

@xxuejie
Copy link
Contributor

xxuejie commented Apr 15, 2024

This part also confuses me: why does the xUDT spec make a choice to use data1(or actually any particular value of hash type?). To me, it is possible to make recommendations, but it should really be the decision of each individual asset to say if it wants to use data1 or type.

And to me it really is wrong to limit hash type to data1 for testnet deployment to get the same value as mainnet.

I get it that one contributing reason, is that the sole deployment of xUDT on mainnet was done without a type ID, but there is really no stopping for one to deploy an xUDT script by themselves with a type ID setup, then issue a new token using type as hash type. Are we saying those tokens are not xUDT tokens?

I do recommend that the spec shall not make decisions on the values of hash type used by an asset type.

@phroi
Copy link
Contributor

phroi commented Apr 21, 2024

The specification was not really clear on xUDT Data, so I dug a bit the code and I was shocked. While the molecule file has a definition for xUDT Data, no validation actually occurs in the xUDT code itself!! 🙀

This claim is easy to verify as:

  • XudtData is not referenced in any other molecule definition.
  • In all generated validating functions the string XudtData always appears in the generated function name.
  • The string XudtData never appears in the xUDT code itself.

After the initial shock, I understood that this is a sensible decision as:

  • To be 100% sUDT compatible xUDT Data rules cannot be possibly enforced in all cases.
  • The XudtData rule enforcement is delegated to the Extension Scripts and Cell Lock who actually need them for locating their own data.
  • In a way rule enforcement works similarly to the CoBuild rules enforcement.
  • This leaves the possibility open for future standard improvements without changing xUDT code itself.

Side note: the molecule definition uses vector Bytes <byte>, but the actual Bytes should be free from such rules. For example no need for the Bytes header if the lock uses a static Struct.

@phroi
Copy link
Contributor

phroi commented Apr 21, 2024

As far as I can see, the intended a molecule representation of XudtData is the following:

// Bytes ...                                  // No Bytes definition as it's a placeholder for anything
option BytesOption (Bytes);                   // BytesOption can be of zero length
vector BytesOptionVec <BytesOption>           // This is a dynvec, some fields may have zero length
option BytesOptionVecOption (BytesOptionVec); // BytesOptionVecOption can be of zero length

table XudtDataTable {
  lock: BytesOption,                          // Possibly Empty
  data: BytesOptionVecOption,                 // Possibly Empty
}

option XudtData (XudtDataTable);

Considerations: in case there are no xUDT Script Extensions and Lock Script stores data as XudtData, the only overhead is the table header, so 12 bytes, is this correct?

@XuJiandong does this represent the intended xUDT behaviour? If so, could you please update the spec to reflect it?

@phroi
Copy link
Contributor

phroi commented Apr 21, 2024

@XuJiandong I was wondering: after the 16 bytes for amount and the XudtData table, is it allowed to have additional data? Or it's forbidden and it should be represented inside the XudtData table?

Example: a NFT data and metadata could be stored after the XudtData table. Does it makes sense or should be forbidden?

@XuJiandong
Copy link
Contributor Author

@phroi All molecule issues can be found in this RFC. Excessive discussion of molecule specifics is not required in this RFC.

@XuJiandong
Copy link
Contributor Author

XuJiandong commented Apr 22, 2024

Example: a NFT data and metadata could be stored after the XudtData table. Does it makes sense or should be forbidden?

It's not allowed. There's no need to store them after the table; they can safely be placed in the item of XudtData.data.

@duanyytop
Copy link
Contributor

The deployment of xUDT on Testnet is TypeId according to the Cryptape blog and almost Dapps/Wallets/Explorer are using the version with TypeId, see https://pudge.explorer.nervos.org/scripts.

I'm confused about why the RFC document ultimately chose version data1, given that very few projects in the ecosystem use this version.

@XuJiandong
Copy link
Contributor Author

XuJiandong commented Jul 17, 2024

The deployment of xUDT on Testnet is TypeId according to the Cryptape blog and almost Dapps/Wallets/Explorer are using the version with TypeId, see https://pudge.explorer.nervos.org/scripts.

I'm confused about why the RFC document ultimately chose version data1, given that very few projects in the ecosystem use this version.

This two versions (data1 and type1) are pointing to the same version of xUDT. It is a history reason. I will update it in RFC.

@phroi
Copy link
Contributor

phroi commented Jul 31, 2024

@duanyytop I'd like to point out that supporting both data1 and type will create issues.

For example a XUDT referenced by type and one referenced by data1 with the same args are two completely different tokens. For example iCKB xUDT will use the data1.

Another issue that may arise is that some platforms will support only type, while other only data1 and some both.

Both these issues will create ecosystem fragmentation:

almost Dapps/Wallets/Explorer are using the version with TypeId

Hence my previous question on the reason of supporting both reference in the RFC.

@xxuejie
Copy link
Contributor

xxuejie commented Aug 1, 2024

For example a XUDT referenced by type and one referenced by data1 with the same args are two completely different tokens. For example iCKB xUDT will use the data1.

Another issue that may arise is that some platforms will support only type, while other only data1 and some both.

Personally, I would disagree with this. To me, it is an unfortunate fact that some platforms only support one value of hash_type (typically type), but I would not personally bet the design on implementation quirks of specific tools.

The deployment of a script code(whether the cell contains the actual code for a script uses type id or not), is actually decoupled from the fact how another will use such script code. The developer of a CKB script code can deploy their code with type ID enabled, but it is not necessarily the fact that others will have to use the type ID. To me it is perfectly normal to use data1 or other data variants to reference a script code deployed with type ID, there is nothing wrong with this pattern.

And yes, the same CKB script code referenced using type and data1 will be treated as 2 different scripts, and this is perfectly okay. Personally, I always consider the combination of code_hash and hash_type to uniquely identify a particular CKB script code. It's really an unfortunate, historical incident, that some platforms only identify a piece of CKB script code via code_hash, and just assume certain value of hash_type.

@phroi
Copy link
Contributor

phroi commented Aug 2, 2024

Since so far seems that we are going for both versions, I asked Lumos if they could integrate the testnet xUDT data1 version, @homura noticed that at 2024-08-02T04:43:05Z there still was no transaction record for the data1 version of the xUDT script. This seems to represent an issue for the integration in Lumos.

@zhangsoledad zhangsoledad merged commit dd4a141 into nervosnetwork:master Nov 12, 2024
1 check passed
@phroi
Copy link
Contributor

phroi commented Nov 12, 2024

Hooray! 🚀🚀🚀

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