Signers: Address post-merge review of #40 — bare compat for /signers, VoidSigner tx population, interface cleanup by claudiovb · Pull Request #95 · tetherto/wdk-wallet-evm

claudiovb · 2026-07-10T20:52:36Z

Description

This PR addresses the post-merge review comments from #40 (EVM: split signer from account). Changes grouped by area:

Bare compatibility (fix)

New src/signers/bare.js entry and a bare condition on the ./signers export. Importing @tetherto/wdk-wallet-evm/signers under the bare runtime previously failed with MODULE_NOT_FOUND: Cannot find module 'http' (the subpath bypassed the bare-node-runtime import mapping applied by the root bare.js); verified fixed against the actual bare runtime.

Transaction population

WalletAccountEvm.sendTransaction now populates transactions with ethers' VoidSigner#populateTransaction instead of our own populator. src/utils/tx-populator-evm.js is deleted entirely and the UnsignedEvmTransaction type is replaced with ethers' TransactionLike. Verified equivalent output (byte-identical signed transactions for legacy, EIP-1559 and EIP-7702 type-4 transactions, including authorization-list gas estimation) and covered by the existing integration suite. One behavioral note: ethers does not auto-detect type-3 from blob fields, so blob transactions require an explicit type: 3 — nothing in the package or tests relied on auto-detection.

Signer interfaces

ISignerEvm moved to its own file src/signers/signer-evm.js and is now exported from the root entry, the /signers barrel, and the types. ISigner is re-exported from the root entry as well.
isDerivable and keyPair moved up to the base ISigner (see companion PR tetherto/wdk-wallet#); ISignerEvm keeps only the EVM-specific members (index, path, address, sign methods) and no longer overrides methods it inherits (derive, getAddress, dispose).
SeedSignerEvm and PrivateKeySignerEvm now @implements the interface instead of extending it, matching the wdk-wallet convention.
Dead guard removed from SeedSignerEvm#index (path/index are always set for a seed signer); JSDoc cleaned up per review (concise descriptions, full specs on all public methods, derive on the private-key signer documented as Promise<never> matching the superclass spec).

Wallet manager

Constructor documented as two overloads (seed / default signer) with @throws tags, and the non-derivable-default-signer error is now a SignerError.
_accounts cache keys simplified to the reviewed scheme: signerName / path / signerName:path.

Tests

tests/signers.test.js split into tests/signers/seed-signer-evm.test.js and tests/signers/private-key-signer-evm.test.js with mirrored test cases for the shared interface surface (constructor, key pair bytes, derive, getAddress, sign, signTransaction, signTypedData, dispose — including double-dispose safety), plus signer-specific cases in each file.
Account/manager/integration tests reverted to the seed+path overloads; the signer overload is covered by a dedicated constructor test using a mock signer (SUT isolated from DOC). Duplicate TRANSACTION constant removed.

Types

All affected .d.ts files hand-edited (no build:types run; generator issues tracked separately): stray generated comments removed, import statements moved to the bottom, SeedSignerEvmOpts.root given a proper structural MemorySafeHDNodeWallet type instead of object (the implementation is internal and ships no declarations).

Verified: 122/122 unit + integration tests (hardhat), standard, tsc over the declaration files, bare-runtime import of both entrypoints, and the wdk-playground e2e suite on both node and bare runtimes against testnet (all checks pass; live sends and transfers go through the new VoidSigner path).

Motivation and Context

Post-merge review of #40 raised valid points that couldn't be addressed before the merge/release: the /signers subpath was not usable under bare, transaction population re-implemented logic ethers already ships (and maintains) in VoidSigner, the ISignerEvm interface lived in the wrong file with redundant overrides and fields that belong on the base ISigner, and the signer test suites and docs needed restructuring. This PR implements the agreed subset of that review; the remaining discussion points are being answered directly on the #40 threads.

Depends on tetherto/wdk-wallet#52 (ISigner.isDerivable/keyPair, IWalletAccount alignment). The @tetherto/wdk-wallet dependency temporarily points at that branch and must be switched to the published version once it's released (as with previous release flows).

Type of change

Bug fix (non-breaking change which fixes an issue)

Davi0kProgramsThings · 2026-07-14T01:56:16Z

+
+export { ISigner } from '@tetherto/wdk-wallet'
+
+export { ISignerEvm } from './src/signers/signer-evm.js'


We should export the 'ISignerEvm' interface only out of /src/signers/index.js.

Davi0kProgramsThings · 2026-07-14T02:28:22Z

+  /**
+   * Signs a message.
+   *
+   * @param {string} message - The message to sign.
+   * @returns {Promise<string>} The message's signature.
+   */
+  async sign (message) {
+    throw new NotImplementedError('sign(message)')
+  }


I think we might be able to move the 'sign' method directly to the 'ISigner' interface since all implementations should be able to sign a message. If this makes sense to you, let's add this change to wdk-wallet pr#52.

Done in wdk-wallet#52 check there when you can

Davi0kProgramsThings · 2026-07-14T02:29:51Z

+/**
+ * Interface for EVM signers, extending the base `ISigner` from `@tetherto/wdk-wallet`.
+ *
+ * @extends {ISigner}


No need to use @extends if the class is already extending the type through the extends keyword.

Davi0kProgramsThings · 2026-07-14T03:06:38Z

  /**
   * The account index. Always undefined for private key signers: a raw key has no
   * BIP-44 position, so reporting an index would be misleading.
+   *
   * @type {number|undefined}
   */
  get index () { return undefined }

  /**
   * The derivation path. Always undefined for private key signers.
+   *
   * @type {string|undefined}
   */
  get path () { return this._path }


The path and index will always be null for a private key signer.

Davi0kProgramsThings · 2026-07-14T03:08:17Z

  /**
   * The derivation path. Always undefined for private key signers.
+   *
   * @type {string|undefined}
   */
  get path () { return this._path }


Private key signers don't need a path property so you can get rid of it:

Suggested change

/**

* The derivation path. Always undefined for private key signers.

*

* @type {string|undefined}

*/

get path () { return this._path }

/**

* The BIP 0044 derivation path.

*

* @type {string | null}

*/

get path () { return null }

Davi0kProgramsThings · 2026-07-14T03:14:16Z

  /**
-   * PrivateKeySignerEvm is not a hierarchical signer and cannot derive.
-   * @returns {Promise<never>}
+   * Derive a child signer using a relative path (e.g., "0'/0/0").
+   *
+   * @param {string} relPath - The relative derivation path.
+   * @returns {Promise<never>} Never resolves; private-key signers cannot derive.
   * @throws {SignerError} Always — private-key signers do not support derivation.
   */
-  async derive () {
+  async derive (relPath) {


#40 (comment)
The specification of the method should be equal to the superclass's except for the return value which should correctly be 'Promise'.

This should be:

Suggested change

/**

* PrivateKeySignerEvm is not a hierarchical signer and cannot derive.

* @returns {Promise<never>}

* Derive a child signer using a relative path (e.g., "0'/0/0").

*

* @param {string} relPath - The relative derivation path.

* @returns {Promise<never>} Never resolves; private-key signers cannot derive.

* @throws {SignerError} Always — private-key signers do not support derivation.

*/

async derive () {

async derive (relPath) {

/**

* Derive a child signer using a relative path (e.g., "0'/0/0").

*

* @param {string} relPath - The relative derivation path.

* @returns {Promise<never>} The derived signer.

* @throws {SignerError} If the signer does not support derivation.

*/

async derive (relPath) {

Davi0kProgramsThings · 2026-07-14T03:15:43Z

Keep the order of the test cases consistent between the two test files.

@extends

- export ISignerEvm only from src/signers/index.js - drop redundant @extends tag on ISignerEvm - path/index return null on PrivateKeySignerEvm (null convention, aligned in ISignerEvm, WalletAccountEvm and wdk-wallet's IWalletAccount) - unify path/index descriptions across signer docs - reorder test cases so both signer suites follow the same order Deferred for the ongoing interface discussion: - moving sign() to ISigner: tetherto#95 (comment) - derive() doc spec alignment: tetherto#95 (comment)

fixes from davide's comments

bdcc93c

claudiovb mentioned this pull request Jul 10, 2026

EVM: Split signer from account #40

Merged

4 tasks

Davi0kProgramsThings requested changes Jul 14, 2026

View reviewed changes

Davi0kProgramsThings assigned claudiovb Jul 14, 2026

claudiovb force-pushed the fix/universal-signer branch from faeb32c to beb66fe Compare July 18, 2026 00:24

address multiple points from davide's comments

11a3d1b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Signers: Address post-merge review of #40 — bare compat for /signers, VoidSigner tx population, interface cleanup#95

Signers: Address post-merge review of #40 — bare compat for /signers, VoidSigner tx population, interface cleanup#95
claudiovb wants to merge 3 commits into
tetherto:mainfrom
claudiovb:fix/universal-signer

claudiovb commented Jul 10, 2026

Uh oh!

Davi0kProgramsThings Jul 14, 2026

Uh oh!

Davi0kProgramsThings Jul 14, 2026

Uh oh!

claudiovb Jul 18, 2026

Uh oh!

Davi0kProgramsThings Jul 14, 2026

Uh oh!

Davi0kProgramsThings Jul 14, 2026

Uh oh!

Davi0kProgramsThings Jul 14, 2026

Uh oh!

Davi0kProgramsThings Jul 14, 2026

Uh oh!

Davi0kProgramsThings Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		export { ISigner } from '@tetherto/wdk-wallet'

		export { ISignerEvm } from './src/signers/signer-evm.js'

Uh oh!

Conversation

claudiovb commented Jul 10, 2026

Description

Motivation and Context

Type of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants