-
Notifications
You must be signed in to change notification settings - Fork 3
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
v2: Simplify setup (no more transactionFactory / messageFactory). #2
Conversation
dc44563
examples/app.js
Outdated
console.log(messageSigned); | ||
} | ||
|
||
class DummyTransaction { |
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.
Oare clasa asta nu ar merge extrasa intr-un fisier separat?
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.
Fixed.
src/extensionProvider.ts
Outdated
|
||
return this.getTransactionFactory().fromPlainObject(txResponse[0]); | ||
async signTransaction<T extends ITransaction>(transaction: T): Promise<T> { | ||
let response = await this.signTransactions([transaction]); |
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.
maybe we should check for nulls here, to not throw an unhandled, we could throw a custom message if the user forgot to include the transaction
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.
The input parameter is not optional. In case of TypeScript code, the user should not be able to compile without passing a transaction.
But you are right about adding improving the error handling / throwing, overall. Is it OK to postpone that to a separate PR?
src/extensionProvider.ts
Outdated
async signTransaction(transaction: ITransaction): Promise<ISignedTransaction> { | ||
const txResponse = await this.startBgrMsgChannel("signTransactions", { | ||
async signTransactions<T extends ITransaction>(transactions: Array<T>): Promise<Array<T>> { | ||
let extensionResponse = await this.startBgrMsgChannel("signTransactions", { |
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.
this looks like a const, as it's not reassigned anywhere
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.
as regarding the startBgrMsgChannel(string), this looks error-prone, maybe we should move these to a constants file and use variables that have strings as values, it's much safer
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.
Fixed.
Fixed, added an enum.
src/extensionProvider.ts
Outdated
}); | ||
|
||
return signedMsg; | ||
const extensionResponse = await this.startBgrMsgChannel("signMessage", 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.
Should we wrap this in a try catch, to avoid throwing back at the user any unhandled exceptions?
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.
Indeed, we should. Is is OK to postpone the improved error handling mechanism to a separate PR?
TransactionFactory
/MessageFactory
anymore.sign()
functions anymore.applySignature()
onITransaction
provider.sendTransaction()
(somehow related to Problem with send transaction logged with maiar app mx-sdk-js-core#201).