Conversation
gringokiwi
commented
Apr 8, 2026
- Ark -> Arkade renamings
- Update boltz-swap and ts-sdk versions
- Replace deprecated functions (e.g. sendBitcoin)
- Use mnemonic instead of single key
- ArkaLightning typo fix
There was a problem hiding this comment.
Code Review — #12
Overall: Straightforward Ark→Arkade rename, SDK version bumps, and API migration. Deprecation aliases for classes/methods are handled well. Three issues need fixing before merge.
🔴 Issue 1: SendParams.feeRate and memo silently dropped
src/skills/arkadeBitcoin.ts:177-185 (post-patch line numbers)
The send() implementation changes from:
await this.wallet.sendBitcoin({ address, amount, feeRate, memo })to:
await this.wallet.send({ address: params.address, amount: params.amount })But SendParams in src/skills/types.ts:15-20 still declares feeRate?: number and memo?: string. Callers passing these fields will silently have them ignored — no error, no warning, money sent with unexpected fee behavior.
Fix: Either (a) remove feeRate and memo from SendParams since the new wallet.send() doesn't support them, or (b) pass them through if wallet.send() accepts them. If removing, mark it in a changelog — it's a breaking interface change.
🟡 Issue 2: Type literal "ark" → "arkade" is a breaking change with no migration path
src/skills/types.ts:11 — BitcoinAddress.type changes from "ark" | "boarding" | "onchain" → "arkade" | "boarding" | "onchain"
src/skills/types.ts:37 — SendResult.type changes from "ark" | "onchain" | "lightning" → "arkade" | "onchain" | "lightning"
Unlike the class renames (ArkaLightningSkill, getArkAddress) which have proper deprecated aliases, these type literal changes have no backward compatibility. Any consumer doing if (result.type === "ark") will silently fail.
Fix: Accept both during a deprecation period:
type: "ark" | "arkade" | "boarding" | "onchain"Or at minimum, document this as a breaking change and bump the minor version accordingly.
🟡 Issue 3: SendParams interface change not reflected in BitcoinSkill
src/skills/types.ts:68-76 — The BitcoinSkill interface still references SendParams which still has feeRate/memo. If these are no longer supported by the underlying SDK, the interface is lying to consumers. The BitcoinSkill interface should also add getArkadeAddress() — which it does in the diff — but confirm the deprecated getArkAddress() is also still there (it is ✓).
✅ What's done well
- Deprecated aliases for
ArkaLightningSkill(const re-export) andgetArkAddress()(method wrapper) — good migration path networkdefault to"bitcoin"— sensible DX improvement, won't break existing callers who passed it explicitly- Docs/examples updated consistently across README, SKILL.md, JSDoc comments
- Lock file is coherent with the version bumps
- No cross-repo breakage risk: no repos in the org directly import
@arkade-os/skill
ℹ️ Not protocol-critical
This PR touches SDK wrappers and naming only — no VTXO handling, signing, forfeit paths, connector trees, or exit logic. Standard review is sufficient.
There was a problem hiding this comment.
Code Review — #12
Overall: Straightforward Ark→Arkade rename, SDK version bumps, and API migration. Deprecation aliases for classes/methods are handled well. Three issues need fixing before merge.
🔴 Issue 1: SendParams.feeRate and memo silently dropped
src/skills/arkadeBitcoin.ts:177-185 (post-patch line numbers)
The send() implementation changes from:
await this.wallet.sendBitcoin({ address, amount, feeRate, memo })to:
await this.wallet.send({ address: params.address, amount: params.amount })But SendParams in src/skills/types.ts:15-20 still declares feeRate?: number and memo?: string. Callers passing these fields will silently have them ignored — no error, no warning, money sent with unexpected fee behavior.
Fix: Either (a) remove feeRate and memo from SendParams since the new wallet.send() doesn't support them, or (b) pass them through if wallet.send() accepts them. If removing, mark it in a changelog — it's a breaking interface change.
🟡 Issue 2: Type literal "ark" → "arkade" is a breaking change with no migration path
src/skills/types.ts:11 — BitcoinAddress.type changes from "ark" | "boarding" | "onchain" → "arkade" | "boarding" | "onchain"
src/skills/types.ts:37 — SendResult.type changes from "ark" | "onchain" | "lightning" → "arkade" | "onchain" | "lightning"
Unlike the class renames (ArkaLightningSkill, getArkAddress) which have proper deprecated aliases, these type literal changes have no backward compatibility. Any consumer doing if (result.type === "ark") will silently fail.
Fix: Accept both during a deprecation period:
type: "ark" | "arkade" | "boarding" | "onchain"Or at minimum, document this as a breaking change and bump the minor version accordingly.
🟡 Issue 3: SendParams interface change not reflected in BitcoinSkill
src/skills/types.ts:68-76 — The BitcoinSkill interface still references SendParams which still has feeRate/memo. If these are no longer supported by the underlying SDK, the interface is lying to consumers. The BitcoinSkill interface should also add getArkadeAddress() — which it does in the diff — but confirm the deprecated getArkAddress() is also still there (it is ✓).
✅ What's done well
- Deprecated aliases for
ArkaLightningSkill(const re-export) andgetArkAddress()(method wrapper) — good migration path networkdefault to"bitcoin"— sensible DX improvement, won't break existing callers who passed it explicitly- Docs/examples updated consistently across README, SKILL.md, JSDoc comments
- Lock file is coherent with the version bumps
- No cross-repo breakage risk: no repos in the org directly import
@arkade-os/skill
ℹ️ Not protocol-critical
This PR touches SDK wrappers and naming only — no VTXO handling, signing, forfeit paths, connector trees, or exit logic. Standard review is sufficient.