-
Notifications
You must be signed in to change notification settings - Fork 124
Utilities and types that let you sign offchain message using the Signers API #991
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: 10-24-add_functions_that_let_you_sign_offchain_message_envelopes
Are you sure you want to change the base?
Conversation
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
20a5a3c to
5ef6547
Compare
c99272f to
e9ef12c
Compare
BundleMonFiles added (3)
Files updated (4)
Unchanged files (126)
Total files change +18.68KB +5.18% Final result: ✅ View report in BundleMon website ➡️ |
e9ef12c to
ff2f77d
Compare
5ef6547 to
c543d03
Compare
c543d03 to
9fbcb37
Compare
fc0222b to
cd7d979
Compare
489ce4d to
fd499f3
Compare
cd7d979 to
97ec70f
Compare
fd499f3 to
c3058bb
Compare
97ec70f to
67fe1e2
Compare
c3058bb to
d105ffc
Compare
7201537 to
abbde28
Compare
d105ffc to
5a6ab95
Compare
|
Documentation Preview: https://kit-docs-n8qfjmv92-anza-tech.vercel.app |
abbde28 to
9f7a067
Compare
5a6ab95 to
312ffe5
Compare
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
lorisleiva
left a comment
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.
Looks very clean! Just a few comments on implementation details.
| Readonly<{ | ||
| address: Address<TAddress>; | ||
| }>; |
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.
Isn't this redundant since MessageSigner<TAddress> already contains { address: Address<TAddress> }?
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've simplified these types in the latest revision. I don't feel like I understand the conceptual underpinning of these types. I've just been trying to copy-paste-modify the account-signer-meta files and repurpose them for requiredSignatories but at most junctures the types don't seem to do what I want them to, let alone something useful.
I think you wrote the original types for account meta signers. Does what I'm doing here make any sense at all?
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've made another comment but this makes sense to me. Though, you could probably simplify this with:
export type OffchainMessageSignatorySigner<TAddress extends string = string> = MessageSigner<TAddress>;| // Ensure there are any MessageModifyingSigner in the first place. | ||
| const modifyingSigners = signers.filter(isMessageModifyingSigner); | ||
| if (modifyingSigners.length === 0) return []; | ||
|
|
||
| // Prefer modifying signers that do not offer partial signing. | ||
| const nonPartialSigners = modifyingSigners.filter(signer => !isMessagePartialSigner(signer)); | ||
| if (nonPartialSigners.length > 0) return nonPartialSigners; | ||
|
|
||
| // Otherwise, choose only one modifying signer (whichever). | ||
| return [modifyingSigners[0]]; |
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.
Perfect!
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 mean, is it? Is it even desirable to permit modifying signers here? Why would you let a signer modify an offchain message, especially one that is destined for multiple signers? Should we even allow modifying signers in the first place?
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 can't think of any use-cases for modifying these messages when signing but there is likely some that I don't see and I think it would be nice to support it for consistency with the rest of the signer API.
Do you have any concerns with this approach?
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.
Only that it's easy to widen the type later, but harder to narrow it. @jordaaash, do you know of any reason why we would or would not allow modifying signers for offchain messages?
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 guess one example is if your v0 message can be displayed with Restricted ASCII but you gave it the message format ASCII, a software wallet that knows it's using a Ledger account might swap the message format before sending it to Ledger.
Also the SIWS spec (or at least Phantom's docs for it) have some fields the wallet is expected to determine if they're not provided, so if you were using this for SIWS then you'd expect the wallet to modify the nested message in those cases.
9f7a067 to
85b9303
Compare
312ffe5 to
438ef45
Compare
85b9303 to
de974db
Compare
438ef45 to
16fa1d8
Compare
de974db to
fb28986
Compare
16fa1d8 to
978e54e
Compare
978e54e to
d411f9b
Compare
fb28986 to
fec9151
Compare
d411f9b to
f25c626
Compare
fec9151 to
623f0d6
Compare
f25c626 to
dacb5f4
Compare
623f0d6 to
a6684ff
Compare
dacb5f4 to
e8781ec
Compare
a6684ff to
4f8008e
Compare
| // Ensure there are any MessageModifyingSigner in the first place. | ||
| const modifyingSigners = signers.filter(isMessageModifyingSigner); | ||
| if (modifyingSigners.length === 0) return []; | ||
|
|
||
| // Prefer modifying signers that do not offer partial signing. | ||
| const nonPartialSigners = modifyingSigners.filter(signer => !isMessagePartialSigner(signer)); | ||
| if (nonPartialSigners.length > 0) return nonPartialSigners; | ||
|
|
||
| // Otherwise, choose only one modifying signer (whichever). | ||
| return [modifyingSigners[0]]; |
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 guess one example is if your v0 message can be displayed with Restricted ASCII but you gave it the message format ASCII, a software wallet that knows it's using a Ledger account might swap the message format before sending it to Ledger.
Also the SIWS spec (or at least Phantom's docs for it) have some fields the wallet is expected to determine if they're not provided, so if you were using this for SIWS then you'd expect the wallet to modify the nested message in those cases.
lorisleiva
left a comment
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.
Took a look at the changes.
| TAddress extends string = string, | ||
| TSigner extends MessageSigner<TAddress> = MessageSigner<TAddress>, | ||
| > { | ||
| requiredSignatories: readonly OffchainMessageSignatorySigner<TAddress, TSigner>[]; |
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.
If I understand correctly, here you want to accept both signers and "address wrappers" as signatories. If so, what you had before makes more sense:
| requiredSignatories: readonly OffchainMessageSignatorySigner<TAddress, TSigner>[]; | |
| requiredSignatories: readonly (OffchainMessageSignatory<TAddress> | OffchainMessageSignatorySigner<TAddress, TSigner>)[]; |
| Readonly<{ | ||
| address: Address<TAddress>; | ||
| }>; |
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've made another comment but this makes sense to me. Though, you could probably simplify this with:
export type OffchainMessageSignatorySigner<TAddress extends string = string> = MessageSigner<TAddress>;e8781ec to
d8b4eb4
Compare
4f8008e to
8d0bb19
Compare
d8b4eb4 to
12f263f
Compare
8d0bb19 to
01762a7
Compare
12f263f to
bb721da
Compare
01762a7 to
2e08910
Compare
bb721da to
0cc9210
Compare
2e08910 to
f24097e
Compare

No description provided.