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
[AIP-X] - Enable interoperability for Federated Keyless Accounts for the same issuer #526
base: main
Are you sure you want to change the base?
[AIP-X] - Enable interoperability for Federated Keyless Accounts for the same issuer #526
Changes from all commits
e7d33f1
abb720b
54cc372
d216427
47563ca
fcccf7c
ea90389
ee9affe
9a810e5
f8a461f
28867a1
851459d
db1f331
46befa7
69ac01a
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.
Can you also describe the changes to the prover service's request format (see https://github.com/aptos-foundation/AIPs/blob/main/aips/aip-75.md#request-format)?
I presume there are no changes to the pepper service, and if so, you should also briefly mention this and maybe discuss why we don't need any changes.
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.
"Developers need to not use
aud
-less accounts when not appropriate" is not helpful.You have to describe when it is and when it isn't appropriate.
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.
Please state more precisely? I assume you are talking about the proving service:
I don't know how you can do such rejection though: the request does not include any information about the address (see here)
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.
"invalidate all existing proofs" --> not very clear what proofs you are referring to.
I think you mean "invalidate all current proofs that are waiting to be sent to the chain by dapps"
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.
Can you please rename this
skip\_aud\_check
? No_val
is needed here. This is just a boolean.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.
Change the color of this bullet to highlight the changes, or put everything in bold? You can keep the old stuff uncolored/unbolded. This will avoid repeating it below like you are doing 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.
IdCommitment
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 not name this additional private input?
skip_aud_checks
This will make it more clear to the reader.
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.
But you have to adjust the language, since here you are confusingly talking about a (non-existant)
dont_skip_aud_checks
input.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.
Again "skip checking the
aud
field" is not precise enough.More precise would be: "skip matching the JWT's
aud
field with theaud
value committed in theIdCommitment
"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 public inputs hash calculation will still implicitly include the
aud
field " --> Yes, but whichaud
field? Again, there are 3 flying around.The same confusion later: "the account requires
aud
to be present"I think you should rewrite this whole paragraph from scratch. Describe everything succinctly and clearly. Don't ever mention an unqualified
aud
field.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 mentioned above, you should show the new API and highlight the changes. This will serve as a good reference for us too.