feat(core): SignerCkbAlwaysSuccess#449
Conversation
Hanssen0
commented
Jul 20, 2026
Hanssen0
commented
- I have read the Contributing Guidelines
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ 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. |
🦋 Changeset detectedLatest commit: bfe4d20 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 |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds and exports ChangesAlways-success CKB signer
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/signer/ckb/signerCkbAlwaysSuccess.test.ts (2)
38-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the transaction returned by the second signing call.
The current assertion checks the input object after discarding
signTransaction’s return value, so it does not directly verify the public API result.Proposed test adjustment
- await signer.signTransaction(signed); - expect(signed.cellDeps).toHaveLength(1); + const signedAgain = await signer.signTransaction(signed); + expect(signedAgain.cellDeps).toHaveLength(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/signer/ckb/signerCkbAlwaysSuccess.test.ts` around lines 38 - 39, Update the test around the second signer.signTransaction call to capture its returned transaction and assert that result’s cellDeps length, rather than asserting the original signed input object. Keep the expected length of 1 unchanged.
5-11: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid making unit tests depend on the public testnet.
ClientPublicTestnetandgetKnownScriptmake these tests dependent on remote availability and deployment metadata. Use a mocked/fake client for unit coverage, or classify and isolate this as an integration test.Also applies to: 28-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/signer/ckb/signerCkbAlwaysSuccess.test.ts` around lines 5 - 11, Update the tests around ClientPublicTestnet, SignerCkbAlwaysSuccess, and getKnownScript to remove public-testnet and remote deployment dependencies. Use a mocked or fake client that supplies the expected AlwaysSuccess script data for unit coverage, or explicitly isolate and classify the test as an integration test if remote behavior must be retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/signer/ckb/signerCkbAlwaysSuccess.test.ts`:
- Around line 38-39: Update the test around the second signer.signTransaction
call to capture its returned transaction and assert that result’s cellDeps
length, rather than asserting the original signed input object. Keep the
expected length of 1 unchanged.
- Around line 5-11: Update the tests around ClientPublicTestnet,
SignerCkbAlwaysSuccess, and getKnownScript to remove public-testnet and remote
deployment dependencies. Use a mocked or fake client that supplies the expected
AlwaysSuccess script data for unit coverage, or explicitly isolate and classify
the test as an integration test if remote behavior must be retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 371b4e2b-d141-4509-8f1e-9e0348607265
📒 Files selected for processing (4)
.changeset/legal-laws-attend.mdpackages/core/src/signer/ckb/index.tspackages/core/src/signer/ckb/signerCkbAlwaysSuccess.test.tspackages/core/src/signer/ckb/signerCkbAlwaysSuccess.ts
There was a problem hiding this comment.
Pull request overview
This PR introduces a new CKB signer implementation for the well-known always-success lock script, enabling transaction “signing” by ensuring the appropriate cell dependency is present (since the script requires no actual signature).
Changes:
- Added
SignerCkbAlwaysSuccess, aSignersubclass that resolves the always-success lock address and injects its cell dep during transaction preparation. - Added Vitest coverage for address/script construction and for ensuring the cell dep is added (and not duplicated) when signing.
- Exported the new signer from the CKB signer barrel and added a changeset for a minor
@ckb-ccc/corerelease.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/signer/ckb/signerCkbAlwaysSuccess.ts | Implements the always-success signer (address resolution + cell-dep injection). |
| packages/core/src/signer/ckb/signerCkbAlwaysSuccess.test.ts | Adds tests for script args/address and cell-dep behavior during signing. |
| packages/core/src/signer/ckb/index.ts | Re-exports SignerCkbAlwaysSuccess from the CKB signer module. |
| .changeset/legal-laws-attend.md | Declares a minor release for the new signer feature. |
860b191 to
bfe4d20
Compare
