feat(udt): add udt info querying methods #261
Conversation
🦋 Changeset detectedLatest commit: a242ce7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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 @Hanssen0, 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 significantly enhances the UDT (User Defined Token) functionality by introducing comprehensive methods for querying UDT information. It provides utilities to calculate the total balance, capacity, and count of UDT cells associated with a signer, offering both detailed information and a simplified balance-only query. This change improves the usability and efficiency of interacting with UDTs, particularly for wallet displays and account analysis, while also deprecating an older, less specific balance calculation function to promote a more robust and UDT-centric approach.
Highlights
- New UDT Querying Methods: Introduces
calculateInfoandcalculateBalancemethods within theUdtclass to provide comprehensive information (balance, capacity, count) or just the total balance of UDT cells for a given signer. These methods offer flexibility for querying UDT data from either the blockchain or a local indexer cache. - Improved Balance Calculation: Adds a dedicated
Udt.balanceFrommethod for accurately extracting UDT balances from cell output data. This new method provides a more robust and UDT-specific way to interpret balance information, leading to the deprecation of the genericudtBalanceFromin the@ckb-ccc/corepackage. - Code Refactoring and Consistency: Existing UDT-related functions, such as those for calculating input/output information and completing transactions, have been refactored to utilize the new
Udt.balanceFrommethod. This ensures consistency across the codebase and leverages the improved UDT-specific balance calculation logic.
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 new methods for querying UDT information, such as total balance and cell count, and refactors the UDT balance calculation logic into the @ckb-ccc/udt package. The changes are well-structured and the new methods are thoroughly documented. My feedback focuses on improving the examples in the documentation to use safer methods for handling large numbers, preventing potential precision loss.
packages/udt/src/udt/index.ts
Outdated
| * const balanceInTokens = Number(info.balance) / (10 ** 8); // Assuming 8 decimals | ||
| * console.log(`Balance: ${balanceInTokens} tokens in ${info.count} cells`); |
There was a problem hiding this comment.
Converting info.balance to Number can lead to precision loss if the balance is very large (greater than Number.MAX_SAFE_INTEGER). It's safer to use a utility function that handles BigInt arithmetic for formatting, like ccc.fixedPointToString.
| * const balanceInTokens = Number(info.balance) / (10 ** 8); // Assuming 8 decimals | |
| * console.log(`Balance: ${balanceInTokens} tokens in ${info.count} cells`); | |
| * const balanceInTokens = ccc.fixedPointToString(info.balance, 8); // Assuming 8 decimals | |
| * console.log(`Balance: ${balanceInTokens} tokens in ${info.count} cells`); |
There was a problem hiding this comment.
Augment wrote these docs before. You're smarter than her.
|
@phroi This should allow us to query info about UDT-like cells. One issue left: How should we handle the custom UDT cell while changing? ccc/packages/udt/src/udt/index.ts Lines 1368 to 1371 in 4393803 and ccc/packages/udt/src/udt/index.ts Line 1435 in 4393803 |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hanssen0
force-pushed
the
feat/udt-query
branch
from
39dee93 to
f2b6d6d
Compare
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new methods for querying UDT information, such as total balance and capacity. The changes include deprecating the old udtBalanceFrom function in favor of a new Udt.balanceFrom method, and adding calculateInfo and calculateBalance to the Udt class. The refactoring is well-structured and the documentation for the new methods is comprehensive.
I have one suggestion regarding testing for the new functionality to ensure its long-term stability.
| async calculateInfo( | ||
| signer: ccc.Signer, | ||
| options?: { source?: "chain" | "local" | null }, | ||
| ): Promise<{ | ||
| balance: ccc.Num; | ||
| capacity: ccc.Num; | ||
| count: number; | ||
| }> { | ||
| let balance = ccc.Zero; | ||
| let capacity = ccc.Zero; | ||
| let count = 0; | ||
| const isFromLocal = (options?.source ?? "chain") === "local"; | ||
|
|
||
| for await (const cell of isFromLocal | ||
| ? signer.findCells(this.filter) | ||
| : signer.findCellsOnChain(this.filter)) { | ||
| balance += this.balanceFrom(cell.outputData, cell.cellOutput); | ||
| capacity += cell.cellOutput.capacity; | ||
| count += 1; | ||
| } | ||
|
|
||
| return { | ||
| balance, | ||
| capacity, | ||
| count, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The new calculateInfo method and its wrapper calculateBalance are great additions for querying UDT information. To ensure their correctness and prevent future regressions, it would be beneficial to add unit tests for them. Consider covering scenarios such as:
- An account with no UDT cells.
- An account with one UDT cell.
- An account with multiple UDT cells.
- Verifying the behavior for both
'local'and'chain'data sources.
|
This is phase 2 of the UDT master plan:
Back to your comment:
You meant Yeah, I have feelings about those two methods!! 🤣 I don't particularly like Additionally, I don't particularly like the non-static
For example in the case of iCKB Receipts:
Similarly goes for iCKB Deposits:
As outputs (so when depositing), the iCKB Receipt will just track the value of the free CKB of the iCKB Deposit(s). This way the protocol work around the on-chain determinism of L1, which doesn't allow to read the transaction inclusion header at validation time. As inputs (so when withdrawing), the positive Receipt iCKB value (and iCKB UDTs) and negative Deposit iCKB value cancel each other out. Are we able to move somewhat closer to Phroi %36 |
First case. I'm very much aware of CKB RPC methods and parameters. Take for example iCKB, a user may own both UDTs and iCKB Receipt cells, which have types that are completely different from each-other, but for example both cells needs to be accounted in the same UDT value for a transaction. Phroi %11 |
Nah, I mean while giving the change cell. I thought the
It's for convenience, so devs won't need to make a
Sure. If it's not an expected UDT cell, let's return 0.
Of course. Do you think any more info is needed?
Absolutely. We just want to try our best to make things faster.
I'm unsure in what perspective we can make them closer.
Is it good enough for you?
This still makes me struggle, and |
I have two possible solutions:
|
That also means |
You are indeed correct, it is a verb, my bad! I just didn't get the meaning, I'm so sorry!! 🤣
You just add a UDT cell as usual, if for any reason the transaction has a positive burned amount of iCKB. Change cell will be always an UDT, even in iCKB. Receipt only tracks deposits in output, so in a way is a kind of change cell in itself. The iCKB validation at L1 script level is: in_udt_ickb + in_receipts_ickb == out_udt_ickb + in_deposits_ickb
Something like that, with the appropriate warning in the TypeDoc
Another thought that comes up sometimes is that at time two or more cells cells need to be consumed togheter. That said, usually there is one cell holding the value, while the other controls the logic of accessing it, so should be fine as it is.
Yesss, think for example if these methods are implemented in SSRI.
Just async + client + outpoint should be enough
Array has an issue: different scripts may have different uses and purposes, as with iCKB Receipt. If it was a named tuple it would make sense for sub-classes, but not for base. For example, an issue is that consuming each of these cells could be done in a different way, we cannot tell for sure how the Transaction will be modified by consuming a non-UDT cell. iCKB Receipts need header in HeaderDeps, for example.
And all methods using directly
Phroi %38 |
Glad to hear that we don't need more detailed control currently! This should make things easier.
Seems like there is no easy solution for this. Perhaps we could create two Udt subclasses, |
I currently have something along those lines, I need to check the feasibility of Subclassing, but I'm honestly too sleepy now... If you want to try your hand with a commit, as soon as I wake up I'll check the feasibility! Wish you a good sleep, night night 🤗 Phroi |
Hanssen0
force-pushed
the
feat/udt-query
branch
3 times, most recently
from
295258e to
25a1301
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and beneficial refactoring by adding CellAny to represent both on-chain and off-chain cells, which simplifies the core API. It also adds new methods for querying UDT information, improving the library's capabilities. The changes are generally well-implemented and documented. However, I've identified a couple of logical errors in the test files that were introduced during the refactoring. Please address these issues to ensure the tests are correct and provide reliable coverage.
Hanssen0
force-pushed
the
feat/udt-query
branch
from
25a1301 to
9886528
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and beneficial refactoring by adding CellAny to represent both on-chain and off-chain cells, which helps in reducing code duplication and improving clarity. It also adds new, well-documented UDT info querying methods. The changes are consistent and well-tested. I've provided a few suggestions to simplify some of the new implementations for better readability and maintainability.
|
#262 should be reviewed first. |
Hanssen0
force-pushed
the
feat/udt-query
branch
from
9886528 to
2ba7fb1
Compare
I suggest that we can safely assume all CellAny with outPoint are inputs, and outputs if without. |
|
What if someone is fetching old transaction and he keeps both inputs and outputs as ccc.Cell to re-validate what happened in that transaction? |
|
Having only the oupoint as un/defined to distinguish between inputs and outputs use is finicky and fragile |
|
Yesterday I was thinking, what if we remove inforFrom and to determine values of single cells we just add the single cell to an empty transaction and from there getInputInfo/getOutputInfo |
|
Using getInputInfo/getOutputInfo then just boils down to expand appropriately the methods interface to include non-tx types? |
|
Alternatively, we need to explicitly pass the intended use input/output to infoFrom |
That means, the dev gives up
That seems to make things more semantically correct. But we can also assume "cells with outpoints are inputs". As I mentioned, if we also assign outPoint to outputs, things will be much more painful. If not, we can do the assumption. |
|
Ok, as reviewer I had to raise the concern! Assumption is reasonable, could you also add some remarks in the TypeDoc? I was wondering, have you considered passing |
At high level, yes, I just need to check deeper the use of two subclasses that you were proposing yesterday 🤔 |
Of course.
Yes. There are two designs: async infoFrom(
client: ccc.Client,
...cells: (ccc.CellAnyLike | ccc.CellAnyLike[])[]
)and async infoFrom(
client: ccc.Client,
cells: ccc.CellAnyLike[],
acc?: InfoLike | null,
)As you can see, the second design incurs some cost: we can no longer pass in a single cell, and that's also true for What do you think? Edit: I rechecked related code, the |
|
We can still pass a single cell by allowing it as param That said, I was mostly looking at optimizing If optimizing a method that should be called as sparingly as possible makes the rest worse, then it makes no sense. I'm not sure myself:
|
Nope. iCKB Receipts are no UDT: they are non transferable, just receipts that can be transformed in UDT or used to withdraw. Basically we are missing all UDT methods. The way I implemented it was:
And maybe I could keep a similar implementation here? Phroi %70 |
@phroi Let's go back here. I'm curious about modifications on What do you think? Or do you suggest also merging some other things? |
|
Epoch would be nice, I worked quite a bit to reach that level. Only issue is that maybe it uses GCD too much. Maybe not all people are comfortable with that. If you add Epoch, then GCD should be added too.
About Maybe also something like unique to extract unique entities from an Iterable? Not sure cause once you implement on Signer the utility to get cells from both local and chain my biggest unique use-case disappears binarySearch and asyncBinarySearch are pretty good utilities too, maybe out of place in CCC Core Phroi %25 |
Hanssen0
force-pushed
the
feat/udt-query
branch
3 times, most recently
from
0eb76e4 to
6a25f02
Compare
Hanssen0
force-pushed
the
feat/udt-query
branch
from
6a25f02 to
a242ce7
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new methods for querying UDT (User Defined Token) information, such as total balance, capacity, and cell count. It adds a new UdtInfo class to encapsulate this data and refactors existing methods to use it, improving code consistency and reusability. The changes are well-documented, including performance warnings for expensive operations. I've provided a few suggestions to further improve performance and readability by optimizing how data is accumulated in some of the new and refactored methods.
| const isFromLocal = (options?.source ?? "chain") === "local"; | ||
|
|
||
| return ccc.reduceAsync( | ||
| isFromLocal |
There was a problem hiding this comment.
Should we add the method on signer to switch between local and chain?
There was a problem hiding this comment.
I think we can do that in another PR
GCD is fine.
I remember that you created a PR once in a while but closed it later. I think we can merge that part into CCC.
I'm thinking that we can add
Considering that they're not currently used by other parts in CCC, now doesn't seem like a good time to add them. However, if they can be designed to be more generic and have full test cases, that's still possible. Considering you are more familiar with these codes, it would be great if you could submit PRs to CCC when you have time. |
|
/canary |
|
🚀 Canary version published successfully! View workflow run The following packages have been published to npm:
|
|
The PR looks pretty good right now, nice job there 👍👍 Just to give you an heads up:
Phroi %13 |
Hanssen0
force-pushed
the
releases/udt
branch
from
bb8a722 to
c0893f1
Compare
2 participants
For full context see #228