Second attempt at UDT Manager #250
Conversation
|
❌ Deploy Preview for liveccc failed. Why did it fail? →
|
❌ Deploy Preview for apiccc failed. Why did it fail? →
|
❌ Deploy Preview for appccc failed. Why did it fail? →
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @phroi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request represents a significant overhaul of the User Defined Token (UDT) management system. The primary goal is to provide a more self-contained and efficient utility for interacting with UDTs on the CKB blockchain. The changes move away from an SSRI-executor-centric approach to a direct, comprehensive management class that handles UDT discovery, balance calculations, and transaction construction, aiming to simplify UDT integration and operations for developers.
Highlights
- Core Class Refactoring: The core
Udtclass has been completely re-architected and renamed toUdtManager, signifying a broader scope of responsibility for managing User Defined Tokens. - SSRI Protocol Decoupling: The previous reliance on the SSRI protocol for UDT metadata and operations has been removed, streamlining UDT management directly within the
UdtManager. - Enhanced UDT Transaction Management: New robust methods have been introduced for finding UDT cells, calculating balances, and comprehensively managing UDT inputs and outputs within transactions, including handling change and ensuring sufficient funds.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the UDT handling logic, replacing the previous SSRI-based Udt class with a new UdtManager. This change makes the UDT logic more self-contained. The new implementation is comprehensive, but I've identified a few issues related to correctness and code clarity. My review includes suggestions to fix a critical bug in input selection, correct a faulty check, and improve the readability of the code by simplifying calculations and renaming a confusing variable.
| output.capacity - initialInput.capacity, | ||
| -(await tx.getFee(client)), | ||
| ); | ||
|
|
There was a problem hiding this comment.
The calculation for minCapacity is confusing. The two arguments passed to ccc.numMin are equivalent, as -(await tx.getFee(client)) resolves to the same value as output.capacity - initialInput.capacity. This makes the use of numMin redundant and the code harder to understand. You can simplify this by using just one of the expressions.
const minCapacity = output.capacity - initialInput.capacity;There was a problem hiding this comment.
False, capacity here is only protocol capacity
There was a problem hiding this comment.
While the one calculated on the transaction is the global one
| } | ||
|
|
||
| const burned = initialInput.balance + addedInput.balance - output.balance; | ||
| if (burned < ccc.Zero) { |
There was a problem hiding this comment.
The variable name burned is misleading. In the context of cryptocurrencies, "burning" usually implies destroying tokens. Here, the variable represents the amount of UDTs to be returned as change. A more descriptive name like changeBalance or balanceDiff would significantly improve code clarity. Please update this variable and its subsequent usages.
const changeBalance = initialInput.balance + addedInput.balance - output.balance;
There was a problem hiding this comment.
Summary of Changes
Hello @phroi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a major refactoring of the User Defined Token (UDT) handling logic. The previous Udt class, which relied on the SSRI protocol for token interactions, has been replaced by a new UdtManager class. This new manager provides a more direct and comprehensive set of utilities for managing UDT cells within transactions, focusing on accurate balance tracking, capacity management, and robust transaction completion. The change aims to provide a more flexible and efficient way to interact with UDTs by moving away from dynamic SSRI execution for core operations.
Highlights
- Class Refactoring and Renaming: The core
Udtclass has been significantly refactored and renamed toUdtManager. This change reflects a shift from a SSRI-dependent UDT implementation to a more generalized and robust UDT management utility. - Removal of SSRI Dependency: The previous reliance on the SSRI protocol for UDT metadata and operations (like
name,symbol,transfer,mint) has been removed. The newUdtManagerfocuses on direct management of UDT cells within transactions. - Enhanced UDT Cell Management: New methods have been introduced to streamline UDT cell handling, including finding cells, calculating balances and capacities, and adding/updating UDT inputs and outputs in transactions. This provides a more granular control over UDT assets.
- Improved Transaction Completion and Error Handling: The transaction completion logic for UDTs has been significantly improved with methods like
completeInputsByBalanceandcompleteChangeTo. These methods ensure that transactions have sufficient UDT balance and capacity, and correctly handle change outputs, including a newErrorUdtInsufficientCoin.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Code Review
This pull request introduces a major refactoring of the UDT handling logic, replacing the SSRI-based Udt class with a new UdtManager. This is a great improvement as it brings more control and transparency to the client-side, removing the dependency on a server-side executor. The new UdtManager provides a comprehensive set of tools for managing UDTs, from finding cells to building complex transactions.
However, my review has identified several critical and high-severity issues in the implementation that need to be addressed. These include incorrect capacity calculation for new UDT outputs, bugs in input gathering logic, and incorrect error reporting for insufficient balances. Please see the detailed comments for each issue. Once these are fixed, this will be a solid foundation for UDT operations.
|
In the end I based it on master, later on we can get back all the other goodies from your PR. OFC the code is not yet ready to be merged, this is more of a proof of concept to gather feedback. @Hanssen0 could you review and point out what you dislike in this approach? |
|
Strange our overlord repeated the same issues two times 🤷♂️ |
Let me quote your comment on Discord to allow others understand what we're talking about better:
|
|
Appreciated you got back to this PR!! 🤗 Others should also check out the previous proposed PR and our comments over there for even more context. @Hanssen0 at high level what do you dislike about how the logic is organized in this PR? (I will fix failing tests, comments and eventual small bugs once we agree that this approach is viable) |
Partially agree. My opinion:
Will UDTs in the future mostly support SSRI? Or should we prefer UDT with SSRI? My answer is yes. Requiring hardcoding UDT info is a historical problem rather than a scenario we should primarily consider. UDT with SSRI should be the native way, rather than a special case of UDT. So, I won't choose the name On the other hand, we do need to deal with "legacy UDTs", and it will be nice if we can reuse code between "legacy UDTs" and "(SSRI) UDTs". So I'm good with names |
I agree that some of them are difficult to understand, like The signature of Another one, the |
Agree. It's better to have more documents than fewer, but not too many. Let's preview the apiccc's final effect and trim down any overly obvious documents. But I think this is a less serious problem. We want future developers to learn how to interact with those APIs without reading the code, so a clear document is more critical than code without a lengthy document that interrupts our reading. |
|
Thank you for all your suggestions! I totally understand that we all want to make things the best we can. However, both the existing #228 and our planned approach are too ambitious to complete in the short term. This dramatically increases the reviewing cost and also the chance of introducing new issues. I suggest that in this PR, we only address the remaining issues in #228 (Sooo sorry I really don't have enough time recently to finish them properly). After we all agree on a working solution, even if it's not perfect, we can merge it first and then create new PRs to improve it further. |
| } | ||
| async *find( | ||
| client: ccc.Client, | ||
| locks: ccc.Script[], |
There was a problem hiding this comment.
I'll always prefer Signer. While it might not be the best name for all cases (Like Watcher or sth might be better than SignerReadonly), it does reduce the aspects that newcomers have to learn. Besides, the utilities implemented in Signer also help us to avoid repeating ourselves.
We should use something like SignerCkbScriptListReadonly instead of Script[] everywhere, for special cases.
There was a problem hiding this comment.
Except Signer does not currently offer any form of findCellsOnChain, so for now it's not a drop in replacement.
Let's say Signer was to offer findCellsOnChain and you also implemented SignerCkbScriptListReadonly, then we could easily change the code to use those newly defined utilities.
| // eslint-disable-next-line @typescript-eslint/require-await | ||
| async infoFrom( | ||
| _client: ccc.Client, | ||
| initialInfo = { |
There was a problem hiding this comment.
I personally dislike this design. My principle is that we should always obtain information from the original Transaction (the original one defined by CKB, not CCC's) if possible. An example is the spore package, where all cobuild things are hidden behind the Transaction.witnesses.
Yes, extra information does make implementation simpler, and it will likely improve performance. However, the increased complexity is too painful. It's a trade-off between performance/simplicity, and usability in most cases.
There was a problem hiding this comment.
@Hanssen0 where in the original PR was mentioned that CoBuild is a target?
we should always obtain information from the original Transaction
Let's break down what it entails:
- inputs: the client is exactly there as initial parameter for obtaining information about the original Transaction, like the inclusion block and Header.
- outputs: we need to update the parameter signature.
...cells: {
cellOutput: ccc.CellOutput;
outputData: ccc.Hex;
outPoint?: ccc.OutPoint;
}[]becomes:
...cells: ccc.Cell[] | {tx : ccc.Transaction, index: number}[]Not really an issue as you can see.
There was a problem hiding this comment.
Let me clarify: the Info is an extra thing that devs must maintain. When using the API, invokers must keep the info along with the tx, even if we can get all we need from the tx. It makes things complex.
There was a problem hiding this comment.
Underlying issue is that CCC doesn't offer a good way to attach additional information to entities like Transaction, Cell and Script... As you know I have been struggling with this since the beginning due to the iCKB headers.
I recently switched to client+cache for the additional information and I was hoping that it would be the end of it.
The CoBuild info would be the object that later on gets serialized into the message?
We can just use a global CCC WeakMap to add this extra info, you just need to modify clone and copy to persist this information across copies
There was a problem hiding this comment.
ccc/packages/spore/src/cobuild/index.ts
Lines 155 to 170 in 705b27d
This is the cobuild approach - it's some additional information that stays with the transaction. We have two ways to do this:
- Let devs maintain the
actionsarray manually. Every new action should be added to the array. After preparing all actions and before completing the transaction, developers should inject/serialize the actions and then fill them back into thetx.witnesses. This is how the old Spore SDK handles cobuild actions. - Hide all cobuild details. Every time our devs perform a new action on a tx, we parse the information from
tx.witnesses, add the action, serialize, and add back to the transaction. This is how we do it now.
The problem here is different from headers caching: we have all we need in the tx; We can parse the cobuild info from tx.witnesses; We can parse the UDT info from tx.inputs / tx.outputs. If we can avoid bothering devs to handle these, we should.
There was a problem hiding this comment.
Besides letting devs focus their mind on a simple tx, this also gives extra compatibility across SDK/ABI/API. Imagine that we're dealing with two devices cooperating to build a transaction. If we have additional information attached to the transaction, we will need to design the ABI carefully to carry it. Alternatively, we need to "pack" this information into the transaction and then "unpack" it on the other end.
Both ways are annoying & complex & leaking abstraction. That's why I insist that it's necessary to avoid attaching information to the transaction.
There was a problem hiding this comment.
Code from Lumos can be seen as another example: fixedEntries and signingEntries. This might work fine at the beginning, while we're still using private keys to sign txs. But soon, as apps need to interact with an increasing number of wallets, things quickly become unmaintainable.
There was a problem hiding this comment.
I'd like to point out that we can add even here whatever we need for CoBuild as optional parameter/property, not really an issue
| if (res) { | ||
| resTx = res.map((res) => ccc.Transaction.fromBytes(res)); | ||
| } | ||
| async updateOutput( |
There was a problem hiding this comment.
Definitely practical method for updating an output's balance!
There was a problem hiding this comment.
Do you have any particular issue in mind?
There was a problem hiding this comment.
No. I don't think this part has any issues. Just want you to know that I think it's a good idea to have this.
There was a problem hiding this comment.
It was there mostly for passing your review. Seems it wasn't enough 🤣
My usual stance would be: do not update an output cell once you already added to the tx, cause you don't know for sure which other cells has been added to the Transaction that depends on the value of the cell that we are trying to modify.
|
The issues you mentioned are a minor code change and we can address them. Any other issue in the code? |
I'm unsure about what you mean by in the code. For me, this currently seems neither completed nor compatible with the original API. I can only provide some suggestions about the basic design. Do you think we should dive deeper into any particular part? |
|
I can see that this PR has zero chances of being merged. Let's get back to your original PR |
2 participants
Closes #227