feat(udt): add udt info querying methods #261
Conversation
🦋 Changeset detectedLatest commit: e8b58dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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.
|
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
| await Promise.resolve(change(tx, balanceBurned, false)), | ||
| ); // Extra capacity introduced by change cell | ||
| tx = ( | ||
| await this.completeInputsByBalance(tx, signer, ccc.Zero, extraCapacity) |
There was a problem hiding this comment.
do we need to force extraCapacity to be positive here? because there is an assumption that it always represents the extra capacity offered by the new change cell.
There was a problem hiding this comment.
Good question. Let's add a check here to notify devs that there might be issues with the change function.
Hanssen0
force-pushed
the
feat/udt-query
branch
2 times, most recently
from
3808149 to
18a9d12
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces valuable methods for querying UDT information, enhancing the library's capabilities. The addition of the UdtInfo class and the refactoring of existing methods for consistency are positive changes. I've identified a couple of opportunities to optimize the performance of the new methods, particularly in how they fetch and process cells, which should improve efficiency for transactions with multiple inputs or outputs.
Hanssen0
force-pushed
the
feat/udt-query
branch
from
18a9d12 to
c31ad19
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new methods for querying UDT information and refactors existing code for better consistency and reusability. The addition of the UdtInfo class is a good abstraction. The refactoring to standardize method signatures with client as the first argument and the use of infoFrom improves code quality. The new features are well-tested. I have one suggestion to improve a comment for better clarity in a complex method.
Hanssen0
force-pushed
the
feat/udt-query
branch
from
c31ad19 to
ae7cbcb
Compare
packages/udt/src/udt/index.ts
Outdated
| * @param infoLike - The `UdtInfoLike` object to add. | ||
| * @returns The current, modified `UdtInfo` instance. | ||
| */ | ||
| addEq(infoLike: UdtInfoLike) { |
There was a problem hiding this comment.
Why the Eq in the name addEq?
There was a problem hiding this comment.
Not an equality after an add, just an assignment to self after an add, better names could be:
- addInPlace
- addMut
- addAssign
- addIn
- addSet
...
There was a problem hiding this comment.
addEq means += and it's shorter...!!!
Ok, I accept this suggestion 😭. addEq is not an explanatory name. I will use addAssign.
| * @internal | ||
| */ | ||
| static balanceFromUnsafe(outputData: ccc.HexLike): ccc.Num { | ||
| const data = ccc.bytesFrom(outputData).slice(0, 16); |
There was a problem hiding this comment.
What happens if there are less than 16 bytes but more than 0?
There was a problem hiding this comment.
By all means also the 0 case shouldn't happen... but it seemingly does, right?
There was a problem hiding this comment.
If the data length is less than 16 bytes, it should be an invalid UDT cell.
So I think we should do
const data = ccc.bytesFrom(outputData).slice(0, 16);
return data.length < 16 ? ccc.Zero : ccc.numFromBytes(data);There was a problem hiding this comment.
Yes, the current behaviour is reasonable because isUdt one way or another is always called before balanceFromUnsafe, so we already are excluding these previously mentioned edge cases within our code
| const isFromLocal = (options?.source ?? "chain") === "local"; | ||
|
|
||
| return ccc.reduceAsync( | ||
| isFromLocal |
There was a problem hiding this comment.
Didn't we add a method already in a separate PR doing this switching internally??
There was a problem hiding this comment.
The method I refer to is the one that internally switch between getCells and getCellsOnchain, probably was on Signer right?
There was a problem hiding this comment.
I checked the code, and this shouldn't have happened yet.
There was a problem hiding this comment.
Yeah, I checked too. Still something to keep in mind casue that pattern is pretty common in my code
|
Once again, I'd like to remind everyone reading that this is phase 2 out of 4 of the CCC UDT master plan:
I reviewed briefly once again, seen a lot of ideas I was working on in my unmerged UDT PR, pretty pleased that you were kind enough to include them back then and now. When I wake up tomorrow I'll review again, Phroi P.S.: I like the idea of a branch dedicated to UDT development, so that we don't have to worry about breaking people until the CCC UDT development is more or less completed 👏👏👏 |
| * @public | ||
| * @category UDT | ||
| */ | ||
| export type UdtInfoLike = |
There was a problem hiding this comment.
I see 4 nulls:
- What's the intended difference in usage
undefinedandnullin this context? - Is the additional complexity worth it?
Maybe add a lil comment documenting the usage if we keep it
There was a problem hiding this comment.
Later on more nulls, not sure if they are actually needed
There was a problem hiding this comment.
This is an attempt to impove developer-experience by relaxing the type restriction. As we all know, JavaScript has two nullish values, undefined and null, and people have mixed them up for a long time. CCC was only accepting undefined at the beginning until we noticed that ckb-sdk-js uses null as an empty value.
Think of this as the design of TypeLike, both null and undefined are EmptyLike and should be accepted in TypeLike types, and they will be converted to undefined in Type.
| return ( | ||
| (ccc.CellOutput.from(cell.cellOutput).type?.eq(this.script) ?? false) && | ||
| (cell.cellOutput.type?.eq(this.script) ?? false) && | ||
| ccc.bytesFrom(cell.outputData).length >= 16 |
There was a problem hiding this comment.
We could improve this recurring pattern for calculating the byte len, how about byteLen(cell.outputData) >= 16
with byteLen defined as:
export function byteLen(data: BytesLike): number {
if (isHex(data)) {
return (data.length - 2) / 2;
}
return bytesFrom(data).length;
}
```
There was a problem hiding this comment.
To be defined in packages/core/src/bytes/index.ts
There was a problem hiding this comment.
I'm happy to see this, but it's better in a separate PR since it might affect many places.
packages/udt/src/udt/index.ts
Outdated
| * signer, | ||
| * ([balanceAcc, capacityAcc], cell) => { | ||
| * const balance = ccc.udtBalanceFrom(cell.outputData); | ||
| * const balance = Udt.balanceFromUnsafe(cell.outputData); |
There was a problem hiding this comment.
In this example where does the isUdt call happen?
There was a problem hiding this comment.
Nice catch! It has been updated.
|
@Hanssen0 LTGM, I don't have additional comments to this PR. I'll do that Speaking of PRs, I just noticed that the Epoch PR was merged in
Phroi %77 |
Let me explain my plan (not ready yet): However, we have already merged some major changes into master a while ago, and I don't want to force-push on it. So I created a We will migrate to the first version after publishing the next major version (cleaning the master branch). My suggestion is to follow the |
Let's try to solve this problem by using polymorphism in the Client. We can have a ClientNoCache or pass a parameter to the constructor to turn off caching, or use |
Gotcha, your WIP plan seems very reasonable!! I'm currently a bit confused due to the transition phase, but I guess that I'll just ask away if any real doubt arise. Glad that you took my Canary feedback to heart! 😁
I like the Thank you, Phroi |
What about |
3 participants
For full context see #228