EVM: Split signer from account #40
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| node_modules | ||
| coverage | ||
| .DS_Store | ||
| .vscode |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,8 +70,9 @@ export default class MemorySafeSigningKey extends SigningKey { | |
| } | ||
|
|
||
| dispose () { | ||
| if (this._privateKeyBuffer) { | ||
|
Contributor
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.
Contributor
Author
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.
Contributor
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()')
}
Contributor
Author
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. |
||
| sodium_memzero(this._privateKeyBuffer) | ||
| this._privateKeyBuffer = undefined | ||
| } | ||
| } | ||
| } | ||
|
Davi0kProgramsThings marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // Copyright 2024 Tether Operations Limited | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // src/signers/index.js | ||
| /** | ||
| * Barrel exports for EVM signers. | ||
| * | ||
| * - `SeedSignerEvm`: derives accounts from a BIP-39 seed (BIP-44 path). | ||
| * - `PrivateKeySignerEvm`: memory-safe wrapper around a raw private key. | ||
| */ | ||
| export { default, default as SeedSignerEvm } from './seed-signer-evm.js' | ||
| export { default as PrivateKeySignerEvm } from './private-key-signer-evm.js' |
Uh oh!
There was an error while loading. Please reload this page.