feat: add channels add/list/fund subcommands - #224
AbdulmalikAlayande merged 2 commits into
Conversation
|
Warning Review limit reached
More reviews will be available in 15 minutes and 7 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a ChangesChannel Account Management
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as channels fund (CLI)
participant Core as fundChannels (core)
participant DB as SQLite (repositories)
participant RPC as StellarRpcClient
User->>CLI: sorokeep channels fund --master-key SK... --amount 10
CLI->>DB: getChannelAccounts(network)
DB-->>CLI: ChannelAccount[]
CLI->>Core: fundChannels(db, masterKey, amount, network, rpcUrl)
Core->>DB: getChannelAccounts(network)
DB-->>Core: ChannelAccount[]
Core->>RPC: sendPayments(destinations, masterKey)
RPC->>RPC: build multi-op XLM transaction
RPC-->>Core: SubmitTransactionResult
Core->>DB: markChannelFunded(publicKey) per account
Core-->>CLI: FundChannelsResult { funded, txHash, errors }
CLI->>User: print success / error output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@mrteeednut007-dotcom Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | 7efd589 | tests/commands/channels.test.ts | View secret |
| - | - | Generic High Entropy Secret | 617e5cd | tests/rpc/client.test.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@src/commands/channels.ts`:
- Around line 92-103: The channels fund handler in fundChannels() only logs
result.errors and still completes successfully, so a failed run is reported as
success. Update the command flow in src/commands/channels.ts so that when
result.errors.length > 0 you terminate with a non-zero exit status or rethrow
after logging the errors, while preserving the existing success logging for
result.funded and result.txHash when funding actually succeeds.
In `@src/core/channels.ts`:
- Around line 16-23: addChannel currently persists any string as public_key, so
malformed keys can later break fundChannels when it builds a transaction from
all stored accounts. Update addChannel in channels.ts to validate the publicKey
argument in core before calling insertChannelAccount, and reject or throw on
invalid keys so non-CLI callers cannot store unusable channel records.
In `@src/db/repositories.ts`:
- Around line 446-448: `markChannelFunded()` is updating rows by public key
only, so it can mark the wrong network’s channel as funded. Update the
repository method signature to accept `network` and use it in the `UPDATE
channel_accounts` query so the row is matched by both `network` and
`public_key`; then follow through any callers of `markChannelFunded()` to pass
the network value consistently.
In `@src/db/schema.sql`:
- Around line 73-80: The channel account uniqueness is currently global on
public_key, which conflicts with network-scoped accounts. Update the
channel_accounts schema to enforce uniqueness on the pair handled by the account
model, using the channel account table definition in schema.sql and the matching
migration logic in src/db/database.ts. Keep the same uniqueness shape in both
places so the add flow for channels can allow the same public key on different
networks without hitting a SQLite constraint error.
In `@tests/commands/channels.test.ts`:
- Around line 179-203: The failed funding test only checks stderr and is missing
the exit-code contract for `channels fund`; update the `registerChannelsCommand`
/ `program.parseAsync` test to also assert the CLI exits non-zero on
`fundChannels` errors, matching the pattern used in the empty-account guard
test. Keep the existing `"Insufficient balance"` stderr assertion, and add the
same rejected `parseAsync()` or `process.exit` expectation so the failure path
in `fundChannels` is fully covered.
- Line 25: The hardcoded secret in the channels test should be removed because
it is a committed credential. Update the test setup in channels.test.ts around
MASTER_KEY to use a generated disposable test key, a mock fixture, or another
non-sensitive value instead of a real Stellar secret. If this value ever
corresponded to a live account, ensure it is rotated or revoked and that any
helper code or constants referencing MASTER_KEY are updated accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7cc7ece4-38d5-4713-8be4-11630c119755
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
package.jsonsrc/commands/channels.tssrc/core/channels.tssrc/db/database.tssrc/db/repositories.tssrc/db/schema.sqlsrc/index.tssrc/rpc/client.tstests/commands/channels.test.ts
📜 Review details
🧰 Additional context used
🪛 Betterleaks (1.5.0)
tests/commands/channels.test.ts
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 54-54: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 77-77: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 99-99: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Check: GitGuardian Security Checks
tests/commands/channels.test.ts
[error] 25-25: GitGuardian detected a hardcoded secret: 'Generic High Entropy Secret' (GitGuardian id referenced in report). Detected in commit 7efd589 at tests/commands/channels.test.ts (diff line R25). Remediate by removing/replacing the secret, storing it securely, and revoking/rotating as appropriate.
🪛 OpenGrep (1.23.0)
tests/commands/channels.test.ts
[WARNING] 25-25: Hardcoded AWS access key detected. Use environment variables or a secrets manager instead.
(coderabbit.secrets.aws-access-key)
🔇 Additional comments (1)
package.json (1)
6-11: LGTM!Also applies to: 49-49
| if (result.errors.length > 0) { | ||
| for (const err of result.errors) { | ||
| console.error(chalk.red(`Error: ${err}`)); | ||
| } | ||
| } | ||
|
|
||
| if (result.funded > 0) { | ||
| console.log(chalk.green(`✔ Funded ${result.funded} channel account(s) successfully.`)); | ||
| if (result.txHash) { | ||
| console.log(` Tx hash: ${chalk.cyan(result.txHash)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return a failing exit code when funding fails.
When fundChannels() returns errors, this handler only prints them and then exits successfully. That makes a failed sorokeep channels fund run look successful to CI and shell scripts.
Exit non-zero after logging the errors (or rethrow) whenever funding fails.
Suggested fix
if (result.errors.length > 0) {
for (const err of result.errors) {
console.error(chalk.red(`Error: ${err}`));
}
+ process.exit(1);
}
if (result.funded > 0) {
console.log(chalk.green(`✔ Funded ${result.funded} channel account(s) successfully.`));
if (result.txHash) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (result.errors.length > 0) { | |
| for (const err of result.errors) { | |
| console.error(chalk.red(`Error: ${err}`)); | |
| } | |
| } | |
| if (result.funded > 0) { | |
| console.log(chalk.green(`✔ Funded ${result.funded} channel account(s) successfully.`)); | |
| if (result.txHash) { | |
| console.log(` Tx hash: ${chalk.cyan(result.txHash)}`); | |
| } | |
| } | |
| if (result.errors.length > 0) { | |
| for (const err of result.errors) { | |
| console.error(chalk.red(`Error: ${err}`)); | |
| } | |
| process.exit(1); | |
| } | |
| if (result.funded > 0) { | |
| console.log(chalk.green(`✔ Funded ${result.funded} channel account(s) successfully.`)); | |
| if (result.txHash) { | |
| console.log(` Tx hash: ${chalk.cyan(result.txHash)}`); | |
| } | |
| } |
🤖 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/commands/channels.ts` around lines 92 - 103, The channels fund handler in
fundChannels() only logs result.errors and still completes successfully, so a
failed run is reported as success. Update the command flow in
src/commands/channels.ts so that when result.errors.length > 0 you terminate
with a non-zero exit status or rethrow after logging the errors, while
preserving the existing success logging for result.funded and result.txHash when
funding actually succeeds.
| export function addChannel( | ||
| db: Database.Database, | ||
| publicKey: string, | ||
| network: string, | ||
| label?: string, | ||
| ): void { | ||
| insertChannelAccount(db, { public_key: publicKey, network, label }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject malformed public keys before persisting them.
addChannel() stores any string as public_key. One bad row here becomes a durable failure for fundChannels(), because the later multi-payment transaction is built from every stored account in that network.
Validate the channel key in core before calling insertChannelAccount() so non-CLI callers cannot persist unusable channel records.
🤖 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/core/channels.ts` around lines 16 - 23, addChannel currently persists any
string as public_key, so malformed keys can later break fundChannels when it
builds a transaction from all stored accounts. Update addChannel in channels.ts
to validate the publicKey argument in core before calling insertChannelAccount,
and reject or throw on invalid keys so non-CLI callers cannot store unusable
channel records.
| export function markChannelFunded(db: Database.Database, publicKey: string): void { | ||
| db.prepare("UPDATE channel_accounts SET funded = 1 WHERE public_key = ?").run(publicKey); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope funded updates by network.
markChannelFunded() updates every row matching the public key, regardless of network. That breaks the network-scoped contract of this feature: funding a testnet channel would also mark the mainnet row as funded for the same address.
Pass network through this repository method and update on (network, public_key) instead.
Suggested fix
-export function markChannelFunded(db: Database.Database, publicKey: string): void {
- db.prepare("UPDATE channel_accounts SET funded = 1 WHERE public_key = ?").run(publicKey);
+export function markChannelFunded(db: Database.Database, publicKey: string, network: string): void {
+ db.prepare(
+ "UPDATE channel_accounts SET funded = 1 WHERE public_key = ? AND network = ?"
+ ).run(publicKey, network);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function markChannelFunded(db: Database.Database, publicKey: string): void { | |
| db.prepare("UPDATE channel_accounts SET funded = 1 WHERE public_key = ?").run(publicKey); | |
| } | |
| export function markChannelFunded(db: Database.Database, publicKey: string, network: string): void { | |
| db.prepare( | |
| "UPDATE channel_accounts SET funded = 1 WHERE public_key = ? AND network = ?" | |
| ).run(publicKey, network); | |
| } |
🤖 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/db/repositories.ts` around lines 446 - 448, `markChannelFunded()` is
updating rows by public key only, so it can mark the wrong network’s channel as
funded. Update the repository method signature to accept `network` and use it in
the `UPDATE channel_accounts` query so the row is matched by both `network` and
`public_key`; then follow through any callers of `markChannelFunded()` to pass
the network value consistently.
| CREATE TABLE IF NOT EXISTS channel_accounts ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| public_key TEXT NOT NULL UNIQUE, | ||
| label TEXT, | ||
| network TEXT NOT NULL DEFAULT 'testnet', | ||
| funded BOOLEAN NOT NULL DEFAULT 0, | ||
| created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make channel uniqueness network-scoped.
public_key TEXT NOT NULL UNIQUE blocks registering the same address on both testnet and mainnet, even though the rest of this feature scopes channel accounts by network. Today that means channels add --network mainnet can bypass the CLI duplicate check and then fail with a raw SQLite constraint error if the same key was already added on testnet.
Use a composite uniqueness constraint on (network, public_key) here, and mirror the same shape in the live migration in src/db/database.ts.
Suggested schema shape
CREATE TABLE IF NOT EXISTS channel_accounts (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- public_key TEXT NOT NULL UNIQUE,
+ public_key TEXT NOT NULL,
label TEXT,
network TEXT NOT NULL DEFAULT 'testnet',
funded BOOLEAN NOT NULL DEFAULT 0,
- created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
+ created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
+ UNIQUE (network, public_key)
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CREATE TABLE IF NOT EXISTS channel_accounts ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| public_key TEXT NOT NULL UNIQUE, | |
| label TEXT, | |
| network TEXT NOT NULL DEFAULT 'testnet', | |
| funded BOOLEAN NOT NULL DEFAULT 0, | |
| created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP | |
| ); | |
| CREATE TABLE IF NOT EXISTS channel_accounts ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| public_key TEXT NOT NULL, | |
| label TEXT, | |
| network TEXT NOT NULL DEFAULT 'testnet', | |
| funded BOOLEAN NOT NULL DEFAULT 0, | |
| created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| UNIQUE (network, public_key) | |
| ); |
🤖 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/db/schema.sql` around lines 73 - 80, The channel account uniqueness is
currently global on public_key, which conflicts with network-scoped accounts.
Update the channel_accounts schema to enforce uniqueness on the pair handled by
the account model, using the channel account table definition in schema.sql and
the matching migration logic in src/db/database.ts. Keep the same uniqueness
shape in both places so the add flow for channels can allow the same public key
on different networks without hitting a SQLite constraint error.
| }); | ||
|
|
||
| describe("channels command", () => { | ||
| const MASTER_KEY = "SCZANGBA5AKIA5OSBZPZU5KA5BWNNASCTLZ5I3XUGP7ZXFJEFZ4MFLN"; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the committed secret key.
This hardcoded Stellar secret is already tripping GitGuardian. Even in tests, checked-in secret seeds need to be treated as compromised. Replace it with a generated disposable test key or a non-sensitive fixture, and revoke/rotate it if it ever backed a real account.
🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 GitHub Check: GitGuardian Security Checks
[error] 25-25: GitGuardian detected a hardcoded secret: 'Generic High Entropy Secret' (GitGuardian id referenced in report). Detected in commit 7efd589 at tests/commands/channels.test.ts (diff line R25). Remediate by removing/replacing the secret, storing it securely, and revoking/rotating as appropriate.
🪛 OpenGrep (1.23.0)
[WARNING] 25-25: Hardcoded AWS access key detected. Use environment variables or a secrets manager instead.
(coderabbit.secrets.aws-access-key)
🤖 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 `@tests/commands/channels.test.ts` at line 25, The hardcoded secret in the
channels test should be removed because it is a committed credential. Update the
test setup in channels.test.ts around MASTER_KEY to use a generated disposable
test key, a mock fixture, or another non-sensitive value instead of a real
Stellar secret. If this value ever corresponded to a live account, ensure it is
rotated or revoked and that any helper code or constants referencing MASTER_KEY
are updated accordingly.
Sources: Linters/SAST tools, Pipeline failures
| it("reports errors from fundChannels", async () => { | ||
| const { fundChannels } = await import("../../src/core/channels.js"); | ||
| (fundChannels as ReturnType<typeof vi.fn>).mockResolvedValueOnce({ | ||
| funded: 0, | ||
| txHash: "", | ||
| errors: ["Insufficient balance"], | ||
| }); | ||
|
|
||
| // Need at least one account so the guard doesn't block | ||
| insertChannelAccount(mockDb, { public_key: "GDQJUTQYK2MQX2VGDR2FYWLIYAQIEGXTQVTFEMGH85FYDNE5VRLJQJN5", network: "testnet" }); | ||
|
|
||
| const program = new Command(); | ||
| registerChannelsCommand(program); | ||
|
|
||
| await program.parseAsync([ | ||
| "node", "sorokeep", | ||
| "channels", "fund", | ||
| "--master-key", MASTER_KEY, | ||
| "--amount", "10", | ||
| "--network", "testnet", | ||
| ]); | ||
|
|
||
| expect(consoleErrorSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining("Insufficient balance") | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the exit-code contract on failed funding.
This case only asserts stderr output. Once the CLI returns a non-zero exit for funding failures, add the same rejected parseAsync() / process.exit expectation used by the empty-account test so the regression stays covered.
🤖 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 `@tests/commands/channels.test.ts` around lines 179 - 203, The failed funding
test only checks stderr and is missing the exit-code contract for `channels
fund`; update the `registerChannelsCommand` / `program.parseAsync` test to also
assert the CLI exits non-zero on `fundChannels` errors, matching the pattern
used in the empty-account guard test. Keep the existing `"Insufficient balance"`
stderr assertion, and add the same rejected `parseAsync()` or `process.exit`
expectation so the failure path in `fundChannels` is fully covered.
2 participants
feat: add channels subcommands — add, list, fund
Adds a sorokeep channels command group for managing channel accounts used for fee bumping and
transaction parallelism.
Subcommands:
channel account
wallet to all registered channel accounts in a single transaction
Implementation:
destination
propagation
Testing:
All 8 new tests pass. Full suite: 235 tests across 14 files, 0 failures.
closes #196