-
Notifications
You must be signed in to change notification settings - Fork 757
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
feat: account abstraction for hash & signature #748
feat: account abstraction for hash & signature #748
Conversation
✅ Deploy Preview for starknetjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I just ended to code an abstracted account contract. |
Tests added. |
Hey @PhilippeR26 ! I think the class would get too many use cases probably. I would rather enforce a good split into private, protected and public methods in the Account class implementation and stay with the standard class way of extending, like
Why do you need such a customizable Account and Signer object? :) |
Many teams are working on Account Abstraction, and their accounts will not be handled with the current code of Starknet.js. |
{ classHash: declare.class_hash, salt, unique, constructorCalldata }, | ||
details | ||
); | ||
const deploy = addsAbstractionDeploy |
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 this ternary operator be avoided and handled in the deployContract
function?
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.
Same.
If not made this way, the compiler requests an iterator in the type BigNumberish[]|undefined
and I don't know how to handle a such case.
): Promise<DeclareDeployUDCResponse> { | ||
const { constructorCalldata, salt, unique } = payload; | ||
let declare = await this.declareIfNot(payload, details); | ||
|
||
let declare = addsAbstractionDeclare |
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 this ternary operator be avoided and handled in the declareIfNot
function?
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 not made this way, the compiler requests an iterator in the type BigNumberish[]|undefined
and I don't know how to handle a such case.
As required, a new |
src/abstractedSigner/default.ts
Outdated
import { AbstractedSignerInterface } from './interface'; | ||
|
||
export class AbstractedSigner extends Signer implements AbstractedSignerInterface { | ||
// constructor() { |
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.
leftover?
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 think you are not in the last commit 8dc277b
src/abstractedSigner/default.ts
Outdated
this.abstractionFunctions = abstractedFns; | ||
} | ||
|
||
// public override transactionSign() { |
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.
leftover?
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 think you are not in the last commit 8dc277b
} from '../types'; | ||
|
||
export abstract class AbstractedSignerInterface { | ||
public abstract abstractionFunctions: AbstractionFunctions | undefined; |
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.
missing docs 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.
In all the project, properties of classes are not documented.
If requested, I can add documentation for the AbstractedSigner class properties.
TypedData, | ||
} from '../types'; | ||
|
||
export abstract class AbstractedSignerInterface { |
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 will leave to others to give input, but as it is currently, this interface has no effect on the AbstractedSigner (the main effect comes from the Signer itself)
): Promise<Invocation> { | ||
const calldata = getExecuteCalldata(call, this.cairoVersion); | ||
const signature = await this.signer.signTransaction(call, signerDetails); | ||
const signature = | ||
'abstractionFunctions' in this.signer |
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 feel like this could be avoided if we expand the original SignerInterface
with optional parameters for addsAbstraction
. cc: @tabaktoni
* @returns response from estimate_fee | ||
*/ | ||
public abstract estimateInvokeFee( | ||
calls: AllowArray<Call>, | ||
estimateFeeDetails?: EstimateFeeDetails | ||
estimateFeeDetails?: EstimateFeeDetails, |
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.
could we push everything related to ...addsAbstraction
inside the estimateFeeDetails
? The same applies for all other methods here
cc: @tabaktoni
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 number of parameters of abstraction are unknown.
Should be replaced by an array.
My feeling is that it will complicate the user experience.
* @returns Add Transaction Response | ||
*/ | ||
public abstract invoke( | ||
method: string, | ||
args?: ArgsOrCalldata, | ||
options?: InvokeOptions | ||
options?: InvokeOptions, | ||
...addsAbstraction: BigNumberish[] |
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.
here also, could we just push stuff through options
?
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.
Same comment. To balance with UX.
nonce, | ||
blockIdentifier, | ||
}); | ||
const adds: string[] = addsAbstraction.map((param) => toHex(param)); |
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.
this line of code is repeated in a lot of places, it would be cool if we could do something different, we can think about it
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.
May be with num.bigNumberishArrayToHexadecimalStringArray()?
version, | ||
nonce, | ||
}); | ||
const signature = |
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.
this could also be avoided if we change the SignerInterface
addressSalt, | ||
constructorCalldata: compiledCalldata, | ||
}); | ||
const signature = |
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.
this could also be avoided if we change the SignerInterface
After releasing v6 final, as c1 gets more stable with c1 acc we can review this one. |
Following Toni request, PR closed. |
Motivation and Resolution
Starknet.js was not able to interact with accounts including account abstraction about the hash and the signature of a transaction.
Source about account abstraction :
Usage related changes
DAPPS coded with Starknet.js v5.19.5, without signature account abstraction, have nothing to change.
The signature abstraction can be implemented in 5 cases. For each case, a function has to be created explaining how to perform this custom hash +signature :
__execute__
These functions have to respect the following signatures :
Example of each case is shown in the guides.
A new function has been implemented :
account.verifyMessageLocally()
for EIP712 like JSON.Nota : no changes have been made in the
ContractFactory
class.Development related changes
Without any modifications in __tests__, tests are passed, meaning that all the new code has not impacted current DAPPS.
A script to test the deployment of a Braavos account, with deploy account abstraction, has been successfully tested.
No tests have been currently implemented. A Cairo v2.1.0 account contract including all cases of signature abstraction will be created and tested.
Probably a new test file will be created for this specific subject.
Checklist: