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?
packages/superise/src/ckb/index.ts
Outdated
|
|
||
| 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.
There was a problem hiding this comment.
We added a prefix for signMessage on the wallet side.
| 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.
There was a problem hiding this comment.
Considering that updating the app is inconvenient, the current wallet design requires signTransaction to manually specify witnessIndexes.
packages/superise/src/ckb/index.ts
Outdated
| 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.
There was a problem hiding this comment.
We added a new signCkbTransaction method, which passes the entire transaction information and displays the transaction details for the user to confirm (similar to JoyID).
|
/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);
}
packages/superise/src/ckb/index.ts
Outdated
| 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"], |
packages/ccc/package.json
Outdated
| "@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 |
The app is still in the testing phase, so we may not be able to provide a version for you to test at the moment. 😂 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the integration of the SupeRISE wallet into the Common Chains Connector (CCC) project. The changes include adding the new @ckb-ccc/superise package, its assets, and integrating its signer functionality into the SignersController. A new verifySuperise.ts file has been added to handle CKB message verification using secp256k1 curves, which appears to be correctly implemented. The overall structure and implementation align with existing patterns in the codebase for wallet integrations.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for the SupeRISE wallet. This includes creating a new package @ckb-ccc/superise, adding the necessary assets and integration points in the main ccc package, and implementing the signing and verification logic in the core package.
My review identified two critical bugs related to signature verification that would cause it to fail. One is in the message hashing logic, and the other is in the signer type returned by the new signer class. I've also provided suggestions for code simplification and fixing a minor typo. A general point of feedback is to ensure consistent naming for "SupeRISE" throughout the codebase (e.g., superise, SupeRISE, SUPERISE) for better maintainability.
| function messageHashCkbSuperise(message: string | BytesLike): Hex { | ||
| const msg = typeof message === "string" ? message : hexFrom(message); | ||
| const buffer = bytesFrom( | ||
| `${SUPERISE_MESSAGE_PREFIX}${msg.length}${msg}`, | ||
| "utf8", | ||
| ); | ||
| return hashCkb(buffer); | ||
| } |
There was a problem hiding this comment.
The implementation of messageHashCkbSuperise is incorrect for BytesLike messages. When message is BytesLike, it's converted to a hex string, and then msg.length is used. This uses the length of the hex string, not the byte length of the message. Additionally, the hex string itself is then encoded as UTF-8, which is not the standard way of hashing byte arrays.
This will cause signature verification to fail for any non-string messages.
The implementation should be similar to messageHashEvmPersonal, where the message is first converted to a Uint8Array to get its byte length, and then the parts are concatenated as bytes.
function messageHashCkbSuperise(message: string | BytesLike): Hex {
const messageBytes = bytesFrom(
message,
typeof message === "string" ? "utf8" : undefined,
);
const prefix = `${SUPERISE_MESSAGE_PREFIX}${messageBytes.length}`;
const prefixBytes = bytesFrom(prefix, "utf8");
const buffer = new Uint8Array(prefixBytes.length + messageBytes.length);
buffer.set(prefixBytes);
buffer.set(messageBytes, prefixBytes.length);
return hashCkb(buffer);
}
There was a problem hiding this comment.
msg.length has different behavior across programming languages. Some use byte length, other might use character count. This might cause problem when developers use different programming languages to interact with SupeRISE wallet. Common pattern is to use byte length as in Bitcoin and Ethereum.
- ccc's messageHashBtcEcdsa
- Ethereum ethers.js hashMessage
| get signType() { | ||
| return ccc.SignerSignType.CkbSecp256k1; | ||
| } |
There was a problem hiding this comment.
The signType getter returns ccc.SignerSignType.CkbSecp256k1. However, a new SupeRISE sign type has been introduced, and the message hashing logic for SupeRISE is different from the standard CKB secp256k1 signing. Returning CkbSecp256k1 will cause Signer.verifyMessage to use verifyMessageCkbSecp256k1, which will fail because of the different message prefix.
This should return ccc.SignerSignType.SupeRISE to ensure the correct verification function (verifyMessageSuperise) is used.
| get signType() { | |
| return ccc.SignerSignType.CkbSecp256k1; | |
| } | |
| get signType() { | |
| return ccc.SignerSignType.SupeRISE; | |
| } |
There was a problem hiding this comment.
Good catch by Gemini here.
There was a problem hiding this comment.
Is returning CkbSecp256k1 intentional? Should this be ccc.SignerSignType.SupeRISE instead? Could you please clarify?
| return secp256k1.verify( | ||
| new secp256k1.Signature( | ||
| numFrom(signatureBytes.slice(0, 32)), | ||
| numFrom(signatureBytes.slice(32, 64)), | ||
| ) | ||
| .addRecoveryBit(Number(numFrom(signatureBytes.slice(64, 65)))) | ||
| .toBytes(), | ||
| bytesFrom(messageHashCkbSuperise(message)), | ||
| bytesFrom(publicKey), | ||
| ); |
There was a problem hiding this comment.
The implementation of verifyMessageSuperise is a bit convoluted. The secp256k1.verify function from @noble/curves can directly accept a 64-byte signature (r and s components). The current code creates a Signature object, adds a recovery bit (which is not used for verification), and then converts it back to a 64-byte Uint8Array.
This can be simplified by slicing the first 64 bytes of the signature and passing it directly to secp256k1.verify. This is more efficient and easier to understand.
return secp256k1.verify(
signatureBytes.slice(0, 64),
bytesFrom(messageHashCkbSuperise(message)),
bytesFrom(publicKey),
);| try { | ||
| this.connection = JSON.parse(connection) as CkbConnection; | ||
| } catch (error) { | ||
| console.error("Failed to restore superise connection:", error); |
There was a problem hiding this comment.
There's a typo in the error message. "superise" should probably be "SupeRISE" to be consistent with the wallet's branding used elsewhere.
| console.error("Failed to restore superise connection:", error); | |
| console.error("Failed to restore SupeRISE connection:", error); |
| return [await this.getAddressObj()]; | ||
| } | ||
|
|
||
| override async signMessageRaw(message: string) { |
There was a problem hiding this comment.
The signMessageRaw method signature should support both string and BytesLike to match the base class signature and other wallet implementations.
| await Promise.all( | ||
| scripts.map(async ({ script, cellDeps }) => { | ||
| await tx.prepareSighashAllWitness(script, 65, this.client); | ||
| await tx.addCellDepInfos(this.client, cellDeps); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Since tx is a shared mutable object, concurrent modifications via Promise.all cause race conditions. We should switch to sequential execution to ensure data integrity.
| private saveConnection() { | ||
| localStorage.setItem( | ||
| this.connectionStorageKey, | ||
| JSON.stringify(this.connection), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Since Superise is an extension wallet, storing the connection in localStorage can cause state inconsistency, like when the user switches accounts. It's better to rely entirely on the bridge's state without persistence.
There was a problem hiding this comment.
Could you help clarify whether the Superise wallet supports address/account switching? If no, we can safely ignore the state inconsistency concern.
Just want to follow up on this: is there now a testable Superise wallet version available, or any alternative testing methods? |
3 participants
