-
-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: update lint tools versions + use ts 5 #55
Conversation
371fbf7
to
2435299
Compare
2435299
to
32704ca
Compare
32704ca
to
aae45b7
Compare
7e43d4b
to
4e2b067
Compare
4e2b067
to
6f9a4a8
Compare
return this.bridge.destroy(); | ||
} | ||
|
||
async serialize() { | ||
async serialize(): Promise<Partial<LedgerBridgeKeyringOptions>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this, but maybe we should have a dedicated type for the state here?
@@ -35,29 +40,36 @@ export class TrezorConnectBridge implements TrezorBridge { | |||
this.trezorConnectInitiated = true; | |||
} | |||
|
|||
dispose() { | |||
async dispose(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not async
but since it's returning a Promise
it makes sense to make it async
@@ -116,14 +125,14 @@ export class TrezorKeyring extends EventEmitter { | |||
return this.bridge.model; | |||
} | |||
|
|||
init() { | |||
async init(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not async
but since it's returning a Promise
it makes sense to make it async
.
Also, the KeyringController
is already await
ing those calls: https://github.com/MetaMask/core/blob/main/packages/keyring-controller/src/KeyringController.ts#L2199-L2201
/** | ||
* EIP-712 Sign Typed Data | ||
*/ | ||
// EIP-712 Sign Typed Data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a simple comment here, since most of those method are using this pattern
c5bda3a
to
54eb2a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did review (but cannot approve since I have some commits here). Left some comments with some open-questions + to explain some of the changes that were made by me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, left a few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits!
c1696c4
to
fff0e11
Compare
Updating
eslint
and other similar packages.