refactor(signer): rename capability Signer to ExternalSigner internally - #225
Merged
Conversation
The capability signer type had three names: declared as Signer in signer/types.ts, imported as AkSigner inside the account classes (to dodge the collision with the legacy Safe owner-identifier union also named Signer), and exported to consumers as ExternalSigner. Align on ExternalSigner everywhere: the declaration is renamed (SignerBase → ExternalSignerBase alongside), the AkSigner import aliases are deleted, and the package root re-exports the name directly instead of aliasing. The legacy Safe union keeps the unqualified Signer name; if it is ever renamed (e.g. SafeOwner), the plain name frees up. Pure rename — no runtime or public-API change (consumers already knew this type only as ExternalSigner). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe SDK renames its account-agnostic signer type to ChangesExternalSigner migration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/account/Calibur/Calibur7702Account.ts (1)
709-721: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep capability-signer documentation aligned with the renamed type.
The code now uses
ExternalSigner, but several signing docs still use the ambiguous legacySignername. Update only capability references; preserveSignerfor the Safe owner-identifier union.
src/account/Calibur/Calibur7702Account.ts#L709-L721: update the accepted-schemes documentation at Line 693 toExternalSigner.src/account/Safe/SafeAccount.ts#L1981-L1981: update capability references at Lines 1949, 1957, 1964, and 1967.src/account/Safe/SafeMultiChainSigAccount.ts#L770-L770: updateSigner<SignContext>at Line 752 and “one Signer per owner” at Line 765.src/account/simple/Simple7702Account.ts#L863-L863: update the accepted-schemes documentation at Line 778.🤖 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 `@src/account/Calibur/Calibur7702Account.ts` around lines 709 - 721, Align signing capability documentation with the renamed ExternalSigner type while preserving Signer for Safe owner-identifier unions. Update the accepted-schemes documentation around signUserOperationWithSigner in src/account/Calibur/Calibur7702Account.ts (lines 709-721), capability references in src/account/Safe/SafeAccount.ts (lines 1949, 1957, 1964, and 1967; anchor 1981), Signer<SignContext> and “one Signer per owner” in src/account/Safe/SafeMultiChainSigAccount.ts (lines 752 and 765; anchor 770), and accepted-schemes documentation in src/account/simple/Simple7702Account.ts (line 778; anchor 863).
🤖 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.
Outside diff comments:
In `@src/account/Calibur/Calibur7702Account.ts`:
- Around line 709-721: Align signing capability documentation with the renamed
ExternalSigner type while preserving Signer for Safe owner-identifier unions.
Update the accepted-schemes documentation around signUserOperationWithSigner in
src/account/Calibur/Calibur7702Account.ts (lines 709-721), capability references
in src/account/Safe/SafeAccount.ts (lines 1949, 1957, 1964, and 1967; anchor
1981), Signer<SignContext> and “one Signer per owner” in
src/account/Safe/SafeMultiChainSigAccount.ts (lines 752 and 765; anchor 770),
and accepted-schemes documentation in src/account/simple/Simple7702Account.ts
(line 778; anchor 863).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21ce3186-e4c8-444c-a69d-8af374aea728
📒 Files selected for processing (12)
src/abstractionkit.tssrc/account/Calibur/Calibur7702Account.tssrc/account/Safe/SafeAccount.tssrc/account/Safe/SafeAccountV0_2_0.tssrc/account/Safe/SafeAccountV0_3_0.tssrc/account/Safe/SafeMultiChainSigAccount.tssrc/account/Safe/adapters.tssrc/account/simple/Simple7702Account.tssrc/account/simple/Simple7702AccountV09.tssrc/signer/adapters.tssrc/signer/negotiate.tssrc/signer/types.ts
…lSigner
Follow-up to the ExternalSigner rename: six doc references in
SafeAccount and SafeMultiChainSigAccount still said Signer while
meaning the capability type — including {@link Signer} links that now
resolved to the legacy Safe owner-identifier union, and a
Signer<SignContext> mention naming a generic that no longer exists.
The unqualified Signer name remains in use only where it genuinely
means the owner union (e.g. owners: Signer[]).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The capability signer type had three names: declared as Signer in signer/types.ts, imported as AkSigner inside the account classes (to dodge the collision with the legacy Safe owner-identifier union also named Signer), and exported to consumers as ExternalSigner.
Align on ExternalSigner everywhere: the declaration is renamed (SignerBase → ExternalSignerBase alongside), the AkSigner import aliases are deleted, and the package root re-exports the name directly instead of aliasing. The legacy Safe union keeps the unqualified Signer name; if it is ever renamed (e.g. SafeOwner), the plain name frees up.
Pure rename — no runtime or public-API change (consumers already knew this type only as ExternalSigner).
Summary by CodeRabbit