Add Cardano support#7
Conversation
| function validateUint32(value: number, detail: string): number { | ||
| if (!Number.isInteger(value) || value < 0 || value > UINT32_MAX) { | ||
| throw invalidTypeError(detail); | ||
| } | ||
| return value; | ||
| } | ||
|
|
||
| function validateUint64(value: bigint, detail: string): bigint { | ||
| if (typeof value !== 'bigint' || value < 0n || value > UINT64_MAX) { | ||
| throw invalidTypeError(detail); | ||
| } | ||
| return value; | ||
| } |
| function bytesArray(bytes: Uint8Array): number[] { | ||
| return Array.from(bytes); | ||
| } |
There was a problem hiding this comment.
Why a helper function that wraps an existing function?
| cardanoSupported(): boolean { | ||
| this.#requireOpen('cardanoSupported'); | ||
| return false; | ||
| return cardanoSupportedImpl(this.#requireOpen('cardanoSupported').info); |
There was a problem hiding this comment.
In ETH, the method is implemented right here in index.ts. Maybe do the same for Cardano for consistency. The Rust-based package uses "self.is_multi_edition()", could make sense to have a similar helper here.
| import { parseKeypath } from '../eth/keypath.js'; | ||
| import { requireVersion } from '../eth/version.js'; |
There was a problem hiding this comment.
Move these helpers to internal/keypath.js and internal/version.js? Not clean to have Cardano (and later BTC) import from eth.
There was a problem hiding this comment.
oops, missed that one
| return Array.from(bytes); | ||
| } | ||
|
|
||
| async function queryCardano( |
There was a problem hiding this comment.
The corresponding ETH function lives in eth/query.ts - change Cardano or ETH to make it consistent? 😄
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
See minor nit, after that feel free to squash and merge
| info: Info, | ||
| transaction: CardanoTransaction, | ||
| ): Promise<CardanoSignTransactionResult> { | ||
| requireCardanoVersion(info); |
There was a problem hiding this comment.
Minor nit: someone on very old firmware will be asked to upgrade to the wrong version (v9.8 instead of v9.22) if tagCborSets is on. They would normally then upgrade to the latest anyway 😄
| # Changelog | ||
|
|
||
| ## [Unreleased] | ||
| - Add Cardano support |
There was a problem hiding this comment.
Please also update the package version to v0.2.0 and move this to a new ## 0.2.0 entry so after merge it's ready for publishing
Move keypath parsing, firmware version checks, product support checks, and integer validation into neutral internal modules. Make Cardano query helper consistent with ETH query layout.
2 participants
No description provided.