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

[FR] Add XUDT testnet reference by data1 #735

Open
phroi opened this issue Aug 1, 2024 · 4 comments
Open

[FR] Add XUDT testnet reference by data1 #735

phroi opened this issue Aug 1, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@phroi
Copy link
Contributor

phroi commented Aug 1, 2024

Lumos should add for XUDT testnetboth reference by type, which is already included and data1.

See:

I just noticed now that Lumos only included the reference by type and not data1 in testnet, the one I assumed it was used. I kid you not, this caused for iCKB quite a mess that I unraveled just a few hours ago.

Lumos references XUDT by type on testnet, while my wrong understanding was that it was referenced by data1 in predefined aggron. My bad. Also the iCKB script assumes reference by data1, not type. Which is okay if the Transaction is set up in the correct way. This means that iCKB XUDT was minted because the receipt existed, but the iCKB XUDT minted (type as predefined by Lumos) was not the one recognized by ICKB script (data1). So basically the Receipt was always fully burned. And a different not compatible not redeemable ICKB XUDT was minted.

By the way for quite some time I was wondering if it was better to change:

    if in_udt_ickb + in_receipts_ickb < out_udt_ickb + in_deposits_ickb {
        return Err(Error::AmountMismatch);
    }

Into:

 if in_udt_ickb + in_receipts_ickb != out_udt_ickb + in_deposits_ickb {
        return Err(Error::AmountMismatch);
    }

to avoid accidental burn and actually it would have prevented this whole mess!! 🤦‍♂️🤣🤣

At any rate, to prevent future tales like mine, please also add a second xUDT entry, with a different name. For example @Keith-CY added xUDT(final_rls).

Additionally iCKB will always reference xUDT by data1, which is a completely different token from one referenced by type, as this tale shows.

Keep up the great work 💪
Phroi

@phroi phroi added the enhancement New feature or request label Aug 1, 2024
@homura
Copy link
Collaborator

homura commented Aug 2, 2024

to avoid accidental burn and actually it would have prevented this whole mess!!

IMO, the change is OK from burnable to unburnable. The main difference between unburnable and burnable tokens is capacity. Unburnable tokens are more secure than burnable ones to prevent accidentally burning tokens. However, this requires users to pay some CKB for its capacity. But I think it's worth it for the CKBs it occupies. For those who really want to burn their tokens, it might be possible to have an app to take the unneeded xUDTs and merge them into a cell, which doesn't seem to cost the user too much

@phroi
Copy link
Contributor Author

phroi commented Aug 2, 2024

IMO, the change is OK from burnable to unburnable.

Actually it's more subtle, I can't and don't intend to prevent burnability in all cases as it would be too complex and overreaching. For iCKB purposes this change should be enough: ickb/v1-core@1647af2#diff-639ebf587e58e8b2881c5c4ce6cc0e7c0d7ec20cf1b88d89511a53a3700f15bd

Any time the iCKB Script executes it prevents burnability, but there still are burns possibilities where the main iCKB Script is not executed, like:

  1. iCKB UDT to iCKB UDT transactions
  2. A transaction that makes iCKB Deposits without minting its receipt. (A iCKB deposit is a Nervos DAO deposit with iCKB Script lock)

Note: I always considered 2) a way to burn capital in a way that is useful. While it effectively burns the capital, it makes for higher availability of deposits to withdraw from for all users.

Fully switching from burnable to unburnable would require:

  1. Logic written in C as extension script of xUDT, which is the base of what you propose.
  2. A way to prevent iCKB deposits without a receipt, which is technically un-faseable as output locks don't validate.
  3. A way to prevent burning in wrapped representations, for example iCKB xUDT is bridged to L2 and burned there, no way to prevent that in all possible wrapped representations.

@homura what's your take on this?

Phroi

@homura
Copy link
Collaborator

homura commented Aug 2, 2024

At any rate, to prevent future tales like mine, please also add a second xUDT entry, with a different name.

I noticed that there is no transaction(2024-08-02T04:43:05Z) record for the data1 version xUDT script. If we include the data1 version in the predefined.TESTNET, it may cause confusion, or as you mentioned in the RFC, these issues will create ecosystem fragmentation. Should we consider treating this as a feature?

@phroi
Copy link
Contributor Author

phroi commented Aug 2, 2024

As it stands now (hopefully within half an hour from 2024-08-02T04:43:05Z) from the RFC both versions on testnet are considered valid. If you feel the RFC can be improved, this is the very best moment to propose a change as the RFC is not published yet. So please feel free to fully read the RFC comment section, the proposed RFC itself and propose a change. I already did it many times and probably I was wrong many more, I'm still a CKB noob 🤷‍♂️

I noticed that there is no transaction(2024-08-02T04:43:05Z) record for the data1 version xUDT script.

@homura if this represents an issue for Lumos, I'll also ask for a transaction record of data1!

Cheers, Phroi

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

No branches or pull requests

2 participants