Skip to content

Commit 4c7af28

Browse files
authored
docs: update add-signer guide for audit remediations and unified factory (#96)
* docs: update add-signer guide and skill for audit remediations and unified factory Align contributor-facing docs with PR #72 (audit remediations) and PR #91 (unified createSigner factory): - Replace sign_partial_transaction with SignTransactionResult enum - Add HTTPS enforcement, HTTP timeout, and HttpClientConfig patterns - Add TransactionUtil shared helpers (classify, add_signature, serialize) - Document error redaction, sanitizeRemoteErrorResponse, safe JSON parsing - Expand TS umbrella package section from 3 to 6 integration files - Update security best practices (zeroize, Option<Pubkey>, no .expect()) * fix(docs): remove unused Duration import, add missing http_config param to test templates
1 parent 6b31ebc commit 4c7af28

File tree

2 files changed

+216
-70
lines changed

2 files changed

+216
-70
lines changed

.claude/skills/add-signer/SKILL.md

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ Orchestrate adding a new signing backend to solana-keychain. Delegate to existin
1818
- **Most recent Rust signer**: `rust/src/para/` (use as pattern)
1919
- **Most recent TS signer**: `typescript/packages/para/` (use as pattern)
2020
- **Trait definition (source of truth)**: `rust/src/traits.rs`
21+
- **Shared HTTP config**: `rust/src/http_client_config.rs`
22+
- **Transaction utilities**: `rust/src/transaction_util.rs`
23+
- **Unified TS factory**: `typescript/packages/keychain/src/create-signer.ts`
24+
- **TS address resolver**: `typescript/packages/keychain/src/resolve-address.ts`
25+
- **TS config union**: `typescript/packages/keychain/src/types.ts`
2126

2227
## Step 1: Gather Information
2328

@@ -54,31 +59,33 @@ Read `docs/ADDING_SIGNERS.md` for detailed code templates. Read `rust/src/para/m
5459
2. Re-export signer type (`pub use <name>::<Name>Signer`)
5560
3. `Signer` enum variant
5661
4. Factory method on `impl Signer` (`from_<name>`)
57-
5. **All 5 match arms** in `impl SolanaSigner for Signer`:
62+
5. **All 4 match arms** in `impl SolanaSigner for Signer`:
5863
- `pubkey()`
5964
- `sign_transaction()`
6065
- `sign_message()`
61-
- `sign_partial_transaction()`
6266
- `is_available()`
6367
6. Add feature to `compile_error!` cfg gate (search for `compile_error!`)
6468

6569
**c) `rust/src/error.rs`** — If signer uses reqwest, add feature to the `#[cfg(any(...))]` gate on `From<reqwest::Error> for SignerError`. Without this, `?` on reqwest calls won't compile.
6670

6771
### Critical Gotchas
6872

69-
- **5 trait methods**: Always read `rust/src/traits.rs` for the current trait definition — it is the source of truth.
70-
- **Return type**: `sign_transaction` and `sign_partial_transaction` return `SignedTransaction = (String, Signature)` — a tuple of base64-encoded transaction + signature.
73+
- **4 trait methods**: Always read `rust/src/traits.rs` for the current trait definition — it is the source of truth. The trait has `pubkey()`, `sign_transaction()`, `sign_message()`, and `is_available()`.
74+
- **Return type**: `sign_transaction` returns `SignTransactionResult` (an enum of `Complete(SignedTransaction)` or `Partial(SignedTransaction)`). Use `TransactionUtil::classify_signed_transaction()` from `rust/src/transaction_util.rs` to classify the result.
75+
- **Shared utilities**: Use `TransactionUtil::add_signature_to_transaction()` and `TransactionUtil::serialize_transaction()` instead of implementing your own.
7176
- **SDK adapter**: Import types from `crate::sdk_adapter`, not `solana_sdk` directly. The project supports both SDK v2 and v3 via an adapter layer.
72-
- **`sign_partial_transaction`**: Serialize with `requireAllSignatures: false`. See existing signers (e.g., `rust/src/para/mod.rs`) for the pattern.
77+
- **HTTPS enforcement**: Remote signers must use `reqwest::ClientBuilder::https_only(true)` gated behind `#[cfg(not(test))]` for wiremock compatibility.
78+
- **HTTP timeouts**: Accept optional `HttpClientConfig` from `crate::http_client_config` for request/connect timeouts (defaults: 30s/5s).
79+
- **Error safety**: Never use `.expect()` or `.unwrap()` on untrusted API responses. Both `Display` and `Debug` on `SignerError` are redacted — keep error messages generic.
7380

7481
### Signer Patterns
7582

7683
**Sync constructor** (public key provided upfront): Memory, Vault, Turnkey, CDP
7784
```
78-
pub fn new(..., public_key: String) -> Result<Self, SignerError>
85+
pub fn new(..., public_key: String, http_config: Option<HttpClientConfig>) -> Result<Self, SignerError>
7986
```
8087

81-
**Async init** (public key fetched from API): Privy, Fireblocks, Dfns, Para
88+
**Async init** (public key fetched from API): Privy, Fireblocks, Dfns, Para, Crossmint
8289
```
8390
pub fn new(...) -> Self // or Result<Self, SignerError>
8491
pub async fn init(&mut self) -> Result<(), SignerError> // fetches pubkey
@@ -89,6 +96,7 @@ let mut signer = <Name>Signer::new(config);
8996
signer.init().await?;
9097
Ok(Self::<Name>(signer))
9198
```
99+
**Important**: Use `Option<Pubkey>` (not `Pubkey::default()`) for the public key field before `init()`. Return `SignerError::ConfigError` from signing methods if `init()` hasn't been called.
92100

93101
**Async constructor** (no separate init step): AWS KMS, GCP KMS
94102
```
@@ -101,14 +109,25 @@ pub struct <Name>SignerConfig { ... }
101109
pub fn new(config: <Name>SignerConfig) -> Self
102110
```
103111

112+
**HTTP client pattern** (all remote signers):
113+
```rust
114+
let http = http_config.unwrap_or_default();
115+
let builder = reqwest::Client::builder()
116+
.timeout(http.resolved_request_timeout())
117+
.connect_timeout(http.resolved_connect_timeout());
118+
#[cfg(not(test))]
119+
let builder = builder.https_only(true);
120+
let client = builder.build()?;
121+
```
122+
104123
## Step 3: Rust Tests
105124

106125
### Unit Tests (wiremock)
107126
Add `#[cfg(test)] mod tests` at the bottom of `mod.rs`. Use `wiremock::MockServer` to mock HTTP endpoints. Cover:
108127
- Constructor validation (valid + invalid inputs)
109128
- `sign_message` success
110-
- `sign_transaction` success
111-
- Error cases (401, malformed response)
129+
- `sign_transaction` success (verify `SignTransactionResult::Complete` vs `Partial`)
130+
- Error cases (401, malformed response) — assert error type only, not error message text
112131
- `is_available` success + failure
113132

114133
### Integration Tests
@@ -144,17 +163,27 @@ Also create: `package.json`, `tsconfig.json`, `README.md`
144163

145164
### Update Umbrella Package: `typescript/packages/keychain/`
146165

147-
- `src/index.ts` — Add namespace export, factory function re-export, and class re-export
166+
6 files to modify:
167+
- `src/types.ts` — Add `YourSignerConfig` to `KeychainSignerConfig` discriminated union with `& { backend: '<name>' }`
168+
- `src/create-signer.ts` — Import `create<Name>Signer`, add switch case
169+
- `src/resolve-address.ts` — Add to fast-path (if config has publicKey) or fetch-path (if async init) switch case
170+
- `src/index.ts` — Add 4 export lines: config type, namespace, factory fn, deprecated class
148171
- `package.json` — Add `@solana/keychain-<name>: "workspace:*"` dependency
149172
- `tsconfig.json` — Add `{ "path": "../<name>" }` reference
150173

174+
The switch statements have exhaustive `never` checks — TypeScript will error if you add to the union but miss a case.
175+
151176
### Key TS Patterns
152177

153178
- Factory function `create<Name>Signer()` returns `SolanaSigner<TAddress>` (the interface)
154179
- Class has `static async create()` method
155180
- Private constructor
156181
- Use `throwSignerError(SignerErrorCode.*, { cause, message })` from `@solana/keychain-core`
157182
- Wrap all `fetch()` calls in try/catch
183+
- Validate `apiBaseUrl` uses HTTPS: `new URL(url).protocol !== 'https:'` → throw `CONFIG_ERROR`
184+
- Sanitize remote error text: `sanitizeRemoteErrorResponse()` from `@solana/keychain-core`
185+
- Guard against malformed JSON: property access inside try/catch or use `?.`
186+
- Add `@throws` JSDoc to factory functions listing error codes
158187

159188
## Step 5: Environment & Docs
160189

0 commit comments

Comments
 (0)