feat(stellar-wallet-snap): add batch proof-of-ownership signing - #267
feat(stellar-wallet-snap): add batch proof-of-ownership signing#267hmalik88 wants to merge 14 commits into
Conversation
There was a problem hiding this comment.
Almost LGTM, Great work,
i especially like the array with length style, i agree we should do it if we have a expected length (will try to copy the pattern to other API later)
It is our of scope of this PR, but shall we also update the singleton request of ProofOfOwnerShip SignProofOfOwnershipJsonRpcRequestStruct to use the shared struct?
Left some nit suggestions that may benefit to all non-evm SNAP
and left a question for the permission
| */ | ||
| export const SignProofOfOwnershipBatchSuccessStruct = object({ | ||
| accountId: UuidStruct, | ||
| signature: pattern(string(), /^0x[0-9a-f]{128}$/u), |
There was a problem hiding this comment.
nit:
lets create a utils for 64Byte0xHexStruct?
as SignProofOfOwnershipJsonRpcResponseStruct also share that patten as well
or you can leave it here, we can move to some share utils later
There was a problem hiding this comment.
Done! 52fea36
I can lift this up to snap-networks-utils later if I see I'm using it elsewhere.
| if (account === undefined) { | ||
| results[index] = { | ||
| accountId, | ||
| error: `Account not found: ${accountId}`, |
There was a problem hiding this comment.
nit:
IMO, we should have a shared message / shared error const or key , as i found that we are doing the same message for other non-evm snap
e.g
results[index] = {
accountId,
error: SignProofOfOwnerShipBatch.AccountNotFound,
}
results[index] = {
accountId,
error: SignProofOfOwnerShipBatch.AccountAddressNotMatch,
}
results[index] = {
accountId,
error: normalizeError(error).message, // custom error
}
...
There was a problem hiding this comment.
Yeah, agreed. We can add this up at the snap-networks-utils level. I can do it in a follow-up PR.
| const signingRequests: SigningRequest[] = []; | ||
|
|
||
| items.forEach(({ accountId, message }, index) => { | ||
| const account = accountsById.get(accountId.toLowerCase()); |
There was a problem hiding this comment.
nit:
looks like we are using account.entropySource as a sign request key
may be we should enforce this attribute not miss?
but i guess this is fine, as when we derive address, we will cross check if the address is same
There was a problem hiding this comment.
I think if there isn't an entropySource in the account then that's more of a data integrity issue, if it's in state it should be guaranteed to have an entropySource.
There was a problem hiding this comment.
i agree, and we already have a "indirect" safe guard later -> assert address check
| signature: add0x(wallet.signMessage(message, 'hex')), | ||
| }; | ||
| } catch (error) { | ||
| results[index] = { |
There was a problem hiding this comment.
just FYI
sign message throw SignMessageException
we can use it to distinguish the error and use different const error key?
There was a problem hiding this comment.
I can do that in a follow up refactor pr using the consts you mentioned above^
|
@metamaskbot publish-preview |
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
stanleyyconsensys
left a comment
stanleyyconsensys
left a comment
There was a problem hiding this comment.
LGTM
there is a sonar cloud alert about account service import multiple time, we can fix it in this PR, or any follow up PR
|
2 participants
Explanation
This PR adds
signProofOfOwnershipBatchsupport to the Stellar Wallet Snap.The new method lets MetaMask request proof-of-ownership signatures for multiple Stellar accounts in one Snap RPC call. Results preserve input order and return per-item success/error objects, so one invalid account or message does not fail the entire batch.
References
N/A
Checklist