EVM: Split signer from account#40
Conversation
Poc ledger signer
sontuphan
left a comment
sontuphan
left a comment
There was a problem hiding this comment.
Hi @claudiovb, I left some comments in the ledger-signer-evm.js. Please have a look when you have time.
fix: bump dmk versions, fix minor bugs, autogen signer types
|
|
| sodium_memzero(this._privateKeyBuffer) | ||
|
|
||
| this._privateKeyBuffer = undefined | ||
| if (this._privateKeyBuffer) { |
There was a problem hiding this comment.
There's no need for the 'dispose' method to be idempotent. By definition, after disposing an object users should expect all methods to have undefined behavior which basically includes anything. We should just guarantee that such undefined behavior doesn't include any unsafe operations but since the private key is not available anymore this is not a possible scenario. These changes are not a mistake but we can safely revert them.
There was a problem hiding this comment.
I'd keep the guard, and I don't think this is about post-dispose undefined behavior... it's about disposal itself being safe to run from multiple owners. WalletManager.dispose() disposes all accounts (each of which disposes its signer) and then loops over the registered signers disposing them again. For a named non-derivable signer, the account's signer is the registered signer object, so manager.dispose() calls dispose() twice on the same object by design. I reproduced this: without the guard the second call crashes with sodium_memzero: buf must be a typed array. So reverting doesn't just make user double-dispose UB; it makes our own teardown path throw.
More generally, idempotent disposal is the standard contract precisely because cleanup graphs overlap: .NET's IDisposable mandates it ("must ignore all calls after the first one") and TC39's DisposableStack.dispose() is specified as a no-op when already disposed. The guard is two lines; I'd rather keep teardown order-independent than make every owner coordinate who disposes first. I also added double-dispose tests to both signer suites in #95 to lock the contract in.
There was a problem hiding this comment.
Good point, this makes sense.
I have a suggestion for a different improvement. I'll post it here though, since i don't think there is a better place. Since the 'dispose' method now exists in multiple interfaces and abstract classes i.e., 'WalletManager', 'IWalletAccount', 'ISigner', it would be better to create an ad-hoc type for it and make them implement it. Your mention to .NET's IDisposable made me remember this improvement i thought about some weeks ago. Feel free to add these changes to wdk-wallet pr#52.
// /src/disposable.js
/** @interface */
export class IDisposable {
/**
* Disposes the object along with all its data (cleaning up any sensitive field from memory).
*/
dispose () {
throw new NotImplementedError('dispose()')
}There was a problem hiding this comment.
Nice idea, done in wdk-wallet#52: added src/disposable.js with IDisposable as you spec'd, ISigner now extends it (its own dispose stub is gone), and IWalletAccount/WalletManager implement it. The concrete signers and accounts needed no changes, they already implement dispose() and inherit the contract through the interface chain.
| const accountSigner = signer.isDerivable | ||
| ? await signer.derive(signer.path.split('/').slice(-3).join('/')) | ||
| : signer | ||
| const account = new WalletAccountEvm(accountSigner, this._config) |
There was a problem hiding this comment.
This should derive the account at the given signer without any further derivation:
| const accountSigner = signer.isDerivable | |
| ? await signer.derive(signer.path.split('/').slice(-3).join('/')) | |
| : signer | |
| const account = new WalletAccountEvm(accountSigner, this._config) | |
| const account = new WalletAccountEvm(signer, this._config) |
There was a problem hiding this comment.
The address is identical either way and deriving the signer's own relative path yields the same account key. The detached child exists for ownership: the account must own key material it's allowed to dispose. With new WalletAccountEvm(signer, ...) handing the registered signer to the account, account.dispose() disposes the registered signer (wiping the root) and every later getAccountByPath(..., { signerName }) fails with "cannot derive child of neutered node". That's precisely the bug Ricardo's reported on a previous review, fixed by this ownership split. Note also that WalletManager.dispose() disposes accounts and then registered signers, so sharing the object would double-dispose it. There's a regression test covering exactly this in the manager suite ("should derive a detached account for a named derivable signer without handing out the root").
There was a problem hiding this comment.
Remember that old discussion about why we are not wiping the seed when the user disposes a wallet manager? The exact same reason applies here as well. Basically, the WalletAccountEvm#dispose method should never wipe external signers. So, if a signer was given at construction we can't dispose it since the class doesn't "own" it and doing so would risk breaking other parts of the application which is exactly what is happening here. If the user creates a new account by providing a seed + path instead of a signer, then it's different since we will create the signer ourself and so we will be the actual owners. Only in this case, when the user calls the 'dispose' method we can wipe the signer. So, what happens is that users should take care of disposing their own signers just like they are already doing with their seeds.
The issue becomes more complex if we take the wallet manager into consideration though. In fact, we also "own" all the signers that we derive when the user's call the WalletManager#getAccount and getAccountByPath methods. So, we need a way to tell the 'WalletAccountEvm' class whether we own the given signer. We can quickly solve this by passing an extra option to its constructor without making it part of the public api i.e., without adding it to the 'EvmWalletConfig' type. In typescript or in other programming languages, this would be the equivalent of adding a constructor overload with package-private visibility that accepts a type that extends 'EvmWalletConfig' with this additional property. However, since we are using javascript we can just pass it and by not adding it to the type definitions it will be invisible to the end user. Basically, in the get account by path method we should change the construction of the account to:
async getAccountByPath (path, options = {}) {
const { signerName } = options
const key = signerName ? `${signerName}:${path}` : path
if (this._accounts[key]) {
return this._accounts[key]
}
const signer = this.getSigner(signerName)
const childSigner = await signer.derive(path)
const account = new WalletAccountEvm(childSigner, { ...this._config, shouldWipeSignerOnDisposal: true })The above works but it is not a really good design since third-party wallet managers wouldn't be able to see and use this property. In order to improve it, we can think about making this field public so that external code can view it as well. This is not bad since it also allows users to delegate our wallet accounts to wipe their own signers. This can be useful if the user knows that the given signer has no other uses in the application and so disposing it along with the account wouldn't break anything. In this case, we shouldn't add it to 'EvmWalletConfig' but rather define it in a new ad-hoc 'SignerOptions' type to accept only in the constructor overload that takes a signer instead of a seed + path:
/**
* @typedef {Object} SignerOptions
* @property {boolean} [shouldWipeSignerOnDisposal] - If true, wipes the signer given at construction on calls to the 'dispose' method.
*/
/** @implements {IWalletAccount} */
export default class WalletAccountEvm extends WalletAccountReadOnlyEvm {
/**
* Creates a new evm wallet account using a signer.
*
* @overload
* @param {ISignerEvm} signer - A signer implementing the EVM signer interface.
* @param {EvmWalletConfig & SignerOptions} [config] - The configuration object.
*/There was a problem hiding this comment.
With these changes, you will be able to apply the above suggestion. Note that we should only enable 'shouldWipeSignerOnDisposal' when the wallet manager derives new signers. When the user calls the get account method by using the overload that just takes a signer name we SHOULDN'T enable the flag, since we will construct the account with a signer that we don't own since it has been given to us by external code. This also applies in the case the user creates the wallet manager with the legacy constructor i.e., by providing a seed.
| async getAddress () { | ||
| if (this._address) return this._address | ||
| const addr = await this._signer.getAddress() | ||
| this.__address = addr |
There was a problem hiding this comment.
Minor mistake here:
| this.__address = addr | |
| this._address = addr |
There was a problem hiding this comment.
This one's intentional: _address is a getter-only accessor on WalletAccountReadOnly, backed by the __address field its constructor sets (line 128). Since ESM is strict mode, this._address = addr throws TypeError: Cannot set property _address of # which has only a getter. __address is the correct write target;
There was a problem hiding this comment.
In the super-class, __address is a private property so it's not really correct to access it in the sub-class. Also, the _address property is optional. This was made to allow sub-classes to not provide a value for it at construction and just provide the address by overriding the get address method. So, this can just be:
/**
* Returns the account's address.
*
* @returns {Promise<string>} The account's address.
*/
async getAddress () {
return await this._signer.getAddress()
}There was a problem hiding this comment.
Good point, but there's one thing we must settle before I remove the address property and drop the __address write inside WalletAccountEvm#getAddress: the public sync address getter on WalletAccountReadOnlyEvm would then return undefined for every WalletAccountEvm (today it's always set, and both our tests and downstream code read it). Do we:
(a) accept that and migrate callers to await getAddress(), keeping the getter meaningful only for read-only accounts constructed with an explicit address ( calling toReadOnlyAccount function) ?
(b) drop the sync getter from the account surface entirely?
| async signAuthorization (auth) { | ||
| return await this._account.authorize(auth) | ||
| const populated = { ...auth } | ||
| if (this._provider) { | ||
| if (populated.chainId == null) { | ||
| const { chainId } = await this._provider.getNetwork() | ||
| populated.chainId = chainId | ||
| } | ||
| if (populated.nonce == null) { | ||
| const address = await this.getAddress() | ||
| populated.nonce = await this._provider.getTransactionCount(address) | ||
| } | ||
| } | ||
| return await this._signer.signAuthorization(populated) | ||
| } |
There was a problem hiding this comment.
c.c. @AlonzoRicardo
Not sure if this is actually necessary.
There was a problem hiding this comment.
It's necessary, and it's a security guard. Our signers aren't provider-connected (by design, the account owns the provider), so ethers can't populate the authorization itself; and BaseWallet.authorizeSync defaults missing fields with auth.chainId || 0 and auth.nonce || 0. Per EIP-7702, a chainId-0 authorization is valid on every chain, so a user calling signAuthorization({ address }) without this populate step would unknowingly sign an any-chain delegation with a likely-wrong nonce. Populating chainId and nonce from the provider before signing is the safe default; explicit values pass through untouched.
There was a problem hiding this comment.
Changing the behavior of a method depending on whether an optional property has been set is generally bad design. So, either we revert this change and put a warning in the documentation or we make the method throw if a provider has not been given at construction.
There was a problem hiding this comment.
Agreed on removing the conditional behavior, implemented the throw variant in #95, with one refinement: it throws when population is actually needed (chainId or nonce missing and no provider), rather than unconditionally when no provider is set. That keeps offline signing with explicit fields working, which is a legitimate flow, while making the dangerous case (signAuthorization({ address }) with no provider, which would have signed a chainId-0 any-chain authorization) a hard error. Both cases are covered by new tests. I thought maybe about an unconditional throw but the current approached seemed better
|
|
||
| test('should sign a transaction and return a valid hex string', async () => { | ||
| const accountWithoutProvider = new WalletAccountEvm(SEED_PHRASE, "0'/0/0") | ||
| const accountWithoutProvider = new WalletAccountEvm(await new SeedSignerEvm(SEED_PHRASE).derive("0'/0/0")) |
There was a problem hiding this comment.
You should revert this change across all test files including integration tests. In order to cover the constructor overload that accepts a signer instead of a seed + path, you should just add a proper test case to the constructor test suite. You should also use a mock instead of an actual implementation since we should always isolate the SUT from any DOC.
There was a problem hiding this comment.
Done in #95 : all test files (unit, manager, integration) are back on the seed+path overloads, and the signer overload is covered by a dedicated constructor test using a mock signer so the SUT is isolated from the DOC. But I kept the single sendTransaction-with-PrivateKeySignerEvm test as real-composition coverage as it's the only test exercising a non-derivable signer through the full account send path.
There was a problem hiding this comment.
But I kept the single sendTransaction-with-PrivateKeySignerEvm test as real-composition coverage as it's the only test exercising a non-derivable signer through the full account send path.
Get rid of this and use an integration test to cover initializating an account with a non-derivable signer.
There was a problem hiding this comment.
Done in #95 , the unit test is gone and the integration suite now covers registering a PrivateKeySignerEvm, getting its account via getAccount(signerName) and sending a transaction end-to-end.
| /** | ||
| * Returns the account's address. If it wasn't resolved at construction time (e.g hardware signers), it asks the | ||
| * underlying signer to resolve it, then caches it locally. | ||
| * | ||
| * @returns {Promise<string>} The account's address. | ||
| */ | ||
| async getAddress () { |
There was a problem hiding this comment.
KISS:
| /** | |
| * Returns the account's address. If it wasn't resolved at construction time (e.g hardware signers), it asks the | |
| * underlying signer to resolve it, then caches it locally. | |
| * | |
| * @returns {Promise<string>} The account's address. | |
| */ | |
| async getAddress () { | |
| /** | |
| * Returns the account's address. | |
| * | |
| * @returns {Promise<string>} The account's address. | |
| */ | |
| async getAddress () { |
There was a problem hiding this comment.
Applied in #95, plus the same treatment on a couple of other doc blocks that described internals (manager's named-signer overload, seed-signer constructor).
|
|
||
| super(account.address, config) | ||
| constructor (signer, config = {}) { | ||
| super(signer.address, config) |
There was a problem hiding this comment.
Since we are removing the 'address' property, here you should change the super constructor call to:
| super(signer.address, config) | |
| super(undefined, config) |
|
|
||
| test('should sign a transaction and return a valid hex string', async () => { | ||
| const accountWithoutProvider = new WalletAccountEvm(SEED_PHRASE, "0'/0/0") | ||
| const accountWithoutProvider = new WalletAccountEvm(await new SeedSignerEvm(SEED_PHRASE).derive("0'/0/0")) |
There was a problem hiding this comment.
But I kept the single sendTransaction-with-PrivateKeySignerEvm test as real-composition coverage as it's the only test exercising a non-derivable signer through the full account send path.
Get rid of this and use an integration test to cover initializating an account with a non-derivable signer.
| async signAuthorization (auth) { | ||
| return await this._account.authorize(auth) | ||
| const populated = { ...auth } | ||
| if (this._provider) { | ||
| if (populated.chainId == null) { | ||
| const { chainId } = await this._provider.getNetwork() | ||
| populated.chainId = chainId | ||
| } | ||
| if (populated.nonce == null) { | ||
| const address = await this.getAddress() | ||
| populated.nonce = await this._provider.getTransactionCount(address) | ||
| } | ||
| } | ||
| return await this._signer.signAuthorization(populated) | ||
| } |
There was a problem hiding this comment.
Changing the behavior of a method depending on whether an optional property has been set is generally bad design. So, either we revert this change and put a warning in the documentation or we make the method throw if a provider has not been given at construction.
There was a problem hiding this comment.
Continuing the discussion about not disposing signers that we don't own, we should also update the implementation of the 'dispose' method both in wdk-wallet and wallet-evm since at the moment we are disposing such components. In the 'WalletManager' abstract class, the 'dispose' method should just take care of disposing its accounts. This will also dispose any signers that we own thanks to the 'shouldWipeSignerOnDisposal' option and the corresponding changes. So, such method becomes just:
// @tetherto/wdk-wallet/src/wallet-manager.js
/** @abstract */
export default class WalletManager {
/**
* Disposes all the wallet accounts, erasing their private keys from the memory.
*/
dispose () {
for (const account of Object.values(this._accounts)) {
if (account.keyPair?.privateKey) {
account.dispose()
}
}
this._accounts = {}
}In the case the user initializes the wallet manager by providing a seed, we would own the resulting seed signer so we need to wipe it on calls to the 'dispose' method. In order to achieve this, we can just override the 'dispose' method in the 'WalletManagerEvm' class:
export default class WalletManagerEvm extends WalletManager {
constructor (seedOrSigner, config = {}) {
[...]
/**
* If true, disposes the default signer on calls to the 'dispose' method.
*
* @protected
* @type {boolean}
*/
this._shouldWipeDefaultSignerOnDisposal = typeof seedOrSigner === 'string' || seedOrSigner instanceof Uint8Array
}
/**
* Disposes all the wallet accounts, erasing their private keys from the memory.
*/
dispose () {
super.dispose()
if (this._shouldWipeDefaultSignerOnDisposal) {
this._defaultSigner.dispose()
}
}4 participants
Description
SeedSignerEvmandPrivateKeySignerEvmas provider-agnostic signers undersrc/signers/, both implementing theISignerEvminterface (which extends the baseISignerfrom@tetherto/wdk-wallet).WalletAccountEvmto accept a signer instead of a raw seed, separating signing logic from provider I/O. Adds astatic fromSeed()andstatic fromPrivateKey()factory (its how to initialise the old way with the seed) and a lazygetAddress()that resolves and caches the address from the signer when it isn't known at construction time.WalletManagerEvmto be signer-based with a backwards-compatibleseedOrSignerconstructor. The default signer must be derivable; non-derivable signers (e.g. private-key) can be registered by name via the inheritedaddSigner()and fetched with the string overload ofgetAccount().getAccountis overloaded: a number selects an account index, a string selects a registered signer.SeedSignerEvmalways exposes an account (index0by default) and can derive children that share the HD root;derive()isasyncto honor the baseISignercontract. Derived children do not retain the root, and the manager never hands the root signer to an account — so disposing an account can never neuter the manager.populateTransactionEvminternal helper to build unsigned transactions across legacy (type 0/1), EIP-1559 (type 2), EIP-4844 (type 3), and EIP-7702 (type 4) styles.signAuthorizationsupport for EIP-7702 delegation on both the seed and private-key signers.EvmTransaction.tooptional and nullable (string | null), enabling contract-creation transactions (omit or passnullforto), matching ethers'TransactionRequest.MemorySafeHDNodeWallet: the child key now derives into a fresh buffer instead of mutating the parent's private key.MemorySafeSigningKey.dispose()to guard against double-dispose../signerssubpath..d.ts) for both signers andpopulateTransactionEvm, and updates the existing declarations (wallet-account-evm.d.ts,wallet-manager-evm.d.ts,wallet-account-read-only-evm.d.ts) to reflect the refactor.PrivateKeySignerEvmcoverage, shared-root disposal regression tests, a contract-creation test, and adapts the constructor/derivecall sites.Motivation and Context
provider.populateTransaction.Type of change
Breaking Changes
WalletAccountEvmconstructor now takes(signer, config)instead of(seed, path, config). UseWalletAccountEvm.fromSeed(seed, path, config)for the old behavior.WalletManagerEvmconstructor acceptsseedOrSigner— raw strings/Uint8Arrayare auto-wrapped into aSeedSignerEvmfor backwards compatibility, but passing a signer directly is now the recommended pattern. The default signer must be derivable.Note
PS: Ledger integration (
LedgerSignerEvm) is intentionally left out of this PR to keep it focused and easy to review. It will come in a separate PR from thepoc-signer-ledgerbranch.