ckb-js / lumos

A full featured dapp framework for Nervos CKB
https://lumos-website.vercel.app/
MIT License
67 stars 54 forks source link

[FR] Add XUDT testnet reference by `data1` #735

Open phroi opened 3 months ago

phroi commented 3 months ago

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

homura commented 3 months ago

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 commented 3 months ago

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: https://github.com/ickb/v1-core/commit/1647af2aadcc5b57bc876fe64c5929dbedf67ea1#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 commented 3 months ago

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 commented 3 months ago

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