feat(core): multiple scripts for SignerCkbScriptReadonly
#265
Conversation
🦋 Changeset detectedLatest commit: 7fba400 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 |
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 introduces a significant enhancement to the SignerCkbScriptReadonly class within the @ckb-ccc/core package. Previously designed to handle a single CKB script, this class can now manage multiple scripts simultaneously. This update allows for more flexible monitoring and address retrieval for several CKB addresses without requiring private keys, improving the utility of the read-only signer for watching purposes.
Highlights
- Multi-Script Support: The SignerCkbScriptReadonly class has been refactored to accept and manage an array of CKB scripts, moving from a single script property to a scripts array.
- Constructor Update: The constructor now accepts a variadic argument of ScriptLike or ScriptLike[], enabling the instantiation of the signer with one or more scripts.
- getAddressObjs Enhancement: The getAddressObjs method now returns an array of Address objects, corresponding to each script managed by the signer, facilitating bulk address retrieval.
- Documentation Refinement: JSDoc comments have been updated to accurately reflect the new multi-script capabilities and clarify the class's purpose as a read-only signer for multiple CKB addresses.
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 apiccc 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 refactors SignerCkbScriptReadonly to support multiple scripts instead of just one. The constructor now accepts an array of scripts or multiple script arguments, and getAddressObjs is updated accordingly. The changes are well-implemented, but there's a potential issue where a SignerCkbScriptReadonly can be created with no scripts, which would lead to runtime errors in other methods. I've added a comment to address this by adding a check in the constructor.
| constructor(client: Client, ...scripts: (ScriptLike | ScriptLike[])[]) { | ||
| super(client); | ||
|
|
||
| this.script = Script.from(script); | ||
| this.scripts = scripts.flat().map(Script.from); | ||
| } |
There was a problem hiding this comment.
The constructor allows creating a SignerCkbScriptReadonly instance with no scripts (e.g., new SignerCkbScriptReadonly(client)). This will cause a runtime error when methods like getInternalAddress() or getRecommendedAddress() are called. These methods from the base Signer class expect getAddressObjs() to return a non-empty array and will fail when trying to access the first element of an empty array.
To prevent this, please add a check to ensure at least one script is provided to the constructor.
constructor(client: Client, ...scripts: (ScriptLike | ScriptLike[])[]) {
super(client);
this.scripts = scripts.flat().map(Script.from);
if (this.scripts.length === 0) {
throw new Error("SignerCkbScriptReadonly requires at least one script.");
}
}
There was a problem hiding this comment.
Alternatively, you can split the first one off, so there should be always one by construction:
constructor(client: Client, script: ScriptLike, ...scripts: ScriptLike[] {Your call
There was a problem hiding this comment.
I considered this approach, but it means we can't use ScriptLike[] as arguments, forcing us to spread the array, which may cause performance issues if the array is large.
There was a problem hiding this comment.
I encountered this issue once when I was working with a byte array. There is a limit on the number of arguments.
There was a problem hiding this comment.
That is true, I encountered it one time too
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Hanssen0
force-pushed
the
feat/signer-multiple-scripts
branch
from
dcfda78 to
7fba400
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances SignerCkbScriptReadonly to support multiple CKB scripts, which is a valuable feature. The implementation is robust, leveraging modern TypeScript features like rest parameters and Array.prototype.flat() to provide a flexible and backward-compatible constructor. The changes correctly propagate to other methods in the Signer hierarchy that depend on getAddressObjs, such as balance checking and transaction finding. The code is clean, well-documented, and includes necessary validation to prevent runtime errors with empty script lists. Overall, this is a high-quality contribution.
|
Just got ready to work!! 🤗 I would say this change makes very much sense and doesn't break anything else 👍 Good work there, |
2 participants
See #250 (comment)
@phroi The Signer with multiple scripts we talked about before.