feat: add superise wallet #330
Conversation
rivjc1025
commented
Nov 18, 2025
rivjc1025
commented
- I have read the Contributing Guidelines
|
✅ 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 liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @rivjc1025, 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 expands the Common Chains Connector's (CCC) wallet support by integrating the SupeRISE wallet. It introduces a new, self-contained package for SupeRISE, complete with a CKB signer implementation, and seamlessly hooks it into the existing CCC framework. This enhancement allows developers and users to leverage SupeRISE for CKB transactions and message signing directly through CCC-powered applications. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
✅ Deploy Preview for docsccc 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.
Code Review
This pull request introduces support for the SupeRISE wallet by adding a new package @ckb-ccc/superise and integrating it into the main ccc package. The new package provides a CkbSigner for interacting with the SupeRISE wallet extension. The changes are well-structured, but I've identified a critical issue with a missing file extension in an import that could lead to runtime errors, along with a few other suggestions to enhance correctness and maintainability.
Hanssen0
left a comment
Hanssen0
left a comment
There was a problem hiding this comment.
Thank you for your contribution! It's happy to see CCC can help devs to integrate the SuperRISE wallet.
This should be a PR that works. However, I have some concerns about the security design of APIs. CCC, or other wallet SDKs, run in an insecure environment, which means code involving security/verification should not depend on the SDK's correctness.
BTW, where can I download the SupeRISE wallet to test this PR?
|
|
||
| override async signMessageRaw(message: string) { | ||
| const signMessage = `Nervos Message:${message}`; | ||
| const sign = await this.bridge.signCkbMessage(signMessage); |
There was a problem hiding this comment.
This may be a security threat: If a website invokes the signCkbMessage API with a transaction hash, users may accidentally sign a tx while only knowing they're signing an arbitrary message.
I recommend adding the message prefix in the wallet to prevent this problem.
| const tx = ccc.Transaction.from(txLike); | ||
|
|
||
| const addressObj = await this.getAddressObj(); | ||
| const acp = await ccc.Script.fromKnownScript( |
There was a problem hiding this comment.
Note: If new scripts are expected to be added in the future, I recommend doing this on the wallet side.
It's harder to let devs update their apps' SDKs than to let users update their wallets, which means we might end up with multiple SDK versions across different apps. As a result, this might cause the new address not to be supported by some apps.
|
|
||
| override async signOnlyTransaction(txLike: ccc.TransactionLike) { | ||
| const tx = ccc.Transaction.from(txLike); | ||
| const metadata = this.getUiMetadataFromTx(tx); |
There was a problem hiding this comment.
interface SignCkbHashAllMetadata {
type: number
nftImage?: string
nftName?: string
toAddress: string
}Please be careful if this metadata is meant to display transaction information for the wallet. The CCC SDK is designed to run in an insecure environment (on the application side), meaning anyone can modify its behavior. Misleading information injected by attackers might lead to users losing their assets.
| if (signatureCache.has(message)) { | ||
| signature = signatureCache.get(message)!; | ||
| } else { | ||
| const sign = await this.bridge.signCkbHashAll( |
There was a problem hiding this comment.
As a recommendation, we usually pass the entire transaction to the wallet to allow it to display the transaction details. Blind signing is always dangerous, and we may want to avoid that.
| return this.signOnlyTransaction(preparedTx); | ||
| } | ||
|
|
||
| override async sendTransaction(tx: ccc.TransactionLike): Promise<ccc.Hex> { |
There was a problem hiding this comment.
It seems unnecessary to override signTransaction and sendTransaction, since they're identical to the default implementations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for the Superise wallet by adding a new package @ckb-ccc/superise and integrating it. The changes are well-structured, but I've found a few issues in the new signer implementation concerning connection state management and transaction signing logic that should be addressed. There is also a minor configuration issue in the documentation generation setup.
| override async disconnect() { | ||
| this.connection = undefined; | ||
| } |
There was a problem hiding this comment.
The disconnect method only clears the in-memory connection property. It should also clear the connection details from localStorage to ensure a consistent disconnected state, especially after a page reload.
override async disconnect() {
this.connection = undefined;
localStorage.removeItem(this.connectionStorageKey);
}| if (!info) { | ||
| return tx; | ||
| } |
There was a problem hiding this comment.
The code returns the transaction object from within the loop if getSignHashInfo returns no info for a script. This will prevent other scripts in the transaction from being signed. You should use continue to proceed to the next script in the loop instead of returning.
if (!info) {
continue;
}| @@ -0,0 +1,6 @@ | |||
| { | |||
| "$schema": "https://typedoc.org/schema.json", | |||
| "entryPoints": ["./src/index.ts", "./src/advanced.ts"], | |||
There was a problem hiding this comment.
The entry point src/advanced.ts does not exist in the package. This will cause an error when generating documentation. Please remove it from the entryPoints array.
| "entryPoints": ["./src/index.ts", "./src/advanced.ts"], | |
| "entryPoints": ["./src/index.ts"], |
| "@ckb-ccc/utxo-global": "workspace:*", | ||
| "@ckb-ccc/xverse": "workspace:*" | ||
| "@ckb-ccc/xverse": "workspace:*", | ||
| "@ckb-ccc/superise": "workspace:*" |
There was a problem hiding this comment.
For better maintainability, it's a good practice to keep dependencies sorted alphabetically. @ckb-ccc/superise should be placed after @ckb-ccc/shell.
| async isConnected() { | ||
| if (this.connection) return true; | ||
|
|
||
| this.restoreConnection(); | ||
| return !!this.connection; | ||
| } |
There was a problem hiding this comment.
The isConnected method has a side effect of trying to restore a connection from localStorage. A method with a name like isConnected is generally expected to be a pure function that only checks the current state without modifying it. This can lead to unexpected behavior. Consider removing the call to restoreConnection() from this method, as the connection restoration logic is already present in getConnection(), which is the more appropriate place for it.
async isConnected() {
return !!this.connection;
}
|
@Hanssen0 |
2 participants
