Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ARC-60 Algorand Wallet Structured Data Signing API #284
base: main
Are you sure you want to change the base?
ARC-60 Algorand Wallet Structured Data Signing API #284
Changes from 12 commits
70dd6d3
e17febd
92ded6a
f33ae41
67754a8
df31e97
7447a0d
8d8cef3
2472440
90a15d7
e92a668
dc9b230
045de9e
a8de5d0
cafda81
4a7173d
c08b995
a33817e
b244ed6
88c3c32
17c149e
282aac4
0285805
f992d9e
43a7178
20ecf74
d379f17
19720ff
5c4184e
4bad240
83a4b79
88923d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
In which cases can
signData
return null? Further below, it is defined that ifsignData
fails, it should throw an error, not return null. So shouldn't the function either returnSigendDataStr
or throw an error?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.
Yes you are right. This is a leftover from a previous design with bulk signatures.
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.
Actually - in the reference implemnetation, I am returning
null
if the user does not confimr the signature: https://github.com/deanstef/ARCs/blob/a8de5d08f3c558bedeb8c1addbacc25362eda7b5/assets/arc-0060/wallet.ts#L79C4-L79C5There 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 this definition,
SigendDataStr
is purely the signature (64 bytes long). In the reference implementation,nacl.sign
is used which returns the signature and the original message.If
SignedDataStr
represents purely the signature, wouldn't something likeSignature
be a more appropriate name for this type?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.
Good catch. Here b244ed6 I updated both the naming and the reference implementation. I am using
nacl.sign.detached
now.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.
Why is the schema (which is a JSON object) represented as a canonicalized JSON string?
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.
In a very first version of ARC60 I was using canonicalized versions of JSON objects to produce signatures of the entire objects. In the new version this is not needed anymore. We can get rid of the canonicalized stuff indeed.
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.
045de9e
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.
What are possible encodings that should be supported?
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.
+1 to clarifying this
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.
It can be any encoding. If the wallet does not support it, it will just reject the call. I make it explicit here 88923d3
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.
In which case could
failingSignData
benull
?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 the error was caused by a missing
StdData
object.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 definition of
SignDataFunction
above, it seems anull
return value is also possible (which I don't think is required as I mentioned earlier).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 table above an invalid schema is a 4601 error code, or is this a different schema?
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.
Error code 4601 means that the JSON schema does not comply with ARC-60 (the required fields
ARC60Domain
andbytes
are missing).In this case, we are failing because
data
is not a valid object according to the schemametadata.schema
.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.
Is this validation here independent of the
ARC60Domain
validation? Because ifARC60Domain
is set toarc60
(or anything other than""
,"TX"
, or"TG"
), wouldn't it be fine ifbytes
started withTX
orTG
? Because the wallet would have to concatenate the value ofARC60Domain
andbytes
and hence no malicious payload could be signed?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.
Setting
ARC60Domain
to""
would make the concatenation not effective. I suggest to always validatebytes
against malicious payloads (prepended withTX
orTG
) to avoid problems.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'm wondering if we should even mention ARC-47 in this ARC here. I'm inclined to remove it for the sake of simplicity and to keep this ARC as short and concise as possible. If there's demand for signing lsigs with ARC-60, somebody can propose a new ARC, specify the scope, and define the schema, etc.
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.
Once the ARC is stable and about to be finalized, it'd be nice to add some test cases. We could specify a keypair, a message (according to the AUTH schema) and show the resulting signature as a test case.
In another test case we could have a keypair derived from a hd-wallet.