Skip to content

Misc updates#12

Merged
gringokiwi merged 29 commits intomasterfrom
james-misc
Apr 14, 2026
Merged

Misc updates#12
gringokiwi merged 29 commits intomasterfrom
james-misc

Conversation

@gringokiwi
Copy link
Copy Markdown
Contributor

  • 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

@gringokiwi gringokiwi requested a review from tiero April 8, 2026 17:54
@gringokiwi gringokiwi requested a review from brg444 April 8, 2026 17:54
Copy link
Copy Markdown

@arkanaai arkanaai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:11BitcoinAddress.type changes from "ark" | "boarding" | "onchain""arkade" | "boarding" | "onchain"

src/skills/types.ts:37SendResult.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) and getArkAddress() (method wrapper) — good migration path
  • network default 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.

Copy link
Copy Markdown

@arkanaai arkanaai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:11BitcoinAddress.type changes from "ark" | "boarding" | "onchain""arkade" | "boarding" | "onchain"

src/skills/types.ts:37SendResult.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) and getArkAddress() (method wrapper) — good migration path
  • network default 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.

@gringokiwi gringokiwi merged commit 0f955bf into master Apr 14, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant