-
Notifications
You must be signed in to change notification settings - Fork 258
Refactor: expose only signBytes
for implementation
#1589
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
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 773491e The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@AlexKushnir1 I agree with the idea that end users shouldn't handle signature verification themselves, yet the current approach still leaves room for mistakes, like forgetting to call one alternative could be to move all the logic into the abstract |
Users should not handle the logic for preparing the message payload.
Hey @denbite! Thanks for pointing out that inaccuracy — it's been fixed. I hope this is the kind of solution you had in mind! |
accountId: accountId, | ||
publicKey: await this.getPublicKey(), | ||
signature: signature, | ||
state: params.callbackUrl, |
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.
@denbite I add state as param.callBackUrl, is that expected logic?
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.
according to the NEP doc, a browser wallet should pass the state
parameter as a query parameter in the callbackUrl
, so we need to parse it out of callbackUrl
and return it to a user if present
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.
IMO it works as it should now.
Previosly the state
was just ignored:
return {
accountId: accountId,
publicKey: pk,
signature: signature,
};
accountId: accountId, | ||
publicKey: await this.getPublicKey(), | ||
signature: signature, | ||
state: params.callbackUrl, |
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.
according to the NEP doc, a browser wallet should pass the state
parameter as a query parameter in the callbackUrl
, so we need to parse it out of callbackUrl
and return it to a user if present
signBytes
for implementation
Pre-flight checklist
pnpm changeset
to create achangeset
JSON document appropriate for this change.Motivation
Provide abstract class
Signer
that has only two methods to implementsignBytes
andgetPublickKey
, while implementing many signature methods. That way, the flow stays consistent, and users only focus on the actual signing stepTest Plan
Added unit tests to verify the correctness of the NEP-413 payload hash generation and signature logic: