-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add keyless account support [AIP-61] #323
Conversation
* Fixed `KeylessSignature` serialization * Fixed `KeylessSignature` serialization
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.
ship itttt
There's 140 commits in this PR, let's try splitting into more PRs in the future. Might also want to look into interactive rebasing w/ squashing, and rebasing on main as opposed to merging. It's extra work but it's important to make it easier for other developers to look at the code, otherwise you might get fewer and less thorough reviews |
getPublicKey(): EphemeralPublicKey { | ||
return this.publicKey; | ||
} |
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 just making publicKey
public readonly?
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.
So @alinush was concerned about the field's misuse and that somebody might think it is the address of an account.
This provides a layer of indirection that makes it harder of confuse (since else where you can access the public key attribute directly), but I also prefer letting it be readonly...
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.
Just to confirm: devs will not be able to feed an EphemeralPublicKey
into a preeexisting call and obtain an address, right?
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 assume we have a type-safe way to derive addresses in the SDK for any of our supported signature schemes?)
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.
Yeah I don't think it's needed, PublicKey
and AccountAddress
are two very different types that are not compatible with each other.
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 long as there's no easy way to convert EphemeralPublicKey
to AccountAddress
in the SDK, we have a much needed layer of protection.
(@hardsetting, it's EphemeralPublicKey
not PublicKey
, right?)
@@ -8,7 +8,7 @@ import { sha3_256 as sha3Hash } from "@noble/hashes/sha3"; | |||
import { RAW_TRANSACTION_SALT, RAW_TRANSACTION_WITH_DATA_SALT } from "../../utils/const"; | |||
import { FeePayerRawTransaction, MultiAgentRawTransaction } from "../instances"; | |||
import { AnyRawTransaction, AnyRawTransactionInstance } from "../types"; | |||
import { Serializable } from "../../bcs"; | |||
import { Serializable } from "../../bcs/serializer"; |
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.
nit: revert
Description
This PR adds KeylessAccount support to the SDK.
There are 2 main additions to the API.
aptos.getPepper and aptos.deriveKeylessAccount.
Most users will ever call aptos.deriveKeylessAccount as it does the work of calling the pepper service.
Basic usage looks like the below
KeylessAccount implements the Account interface and thus may be used in all signing APIs.
aptos.deriveKeylessAccount has several optional parameters
uidKey
- this allows the caller to override the default of 'sub' if the developer so choosespepper
- this allows the caller to manually set the pepper. The pepper service will not be called if this is set.extraFieldKey
- a key on the JWT payload that lets a user publically 'reveal' a field on the chainproofFetchCallback
- if a callback is specified, the proof will be fetched asynchronously and the callback will be invoked on finish. This allows for frontends to display faster and let proof fetching happen in the backgroundTest Plan
An e2e test for submitting a keyless txn is included.
More work to add a local prover/pepper needs to be done to support a true end to end test.
Related Links