Skip to content

feat: splitting signing methods into prepare and sign #1629

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

howydev
Copy link
Collaborator

@howydev howydev commented May 19, 2025

Pull Request Checklist


PR-Codex overview

This PR focuses on enhancing the signature handling in the account-kit smart contracts by introducing new types and methods for managing signature requests. It includes improvements for signing messages and typed data, ensuring better error handling and type safety.

Detailed summary

  • Added SignatureRequest type with specific request types.
  • Introduced SigningMethods type for signature preparation and formatting.
  • Implemented assertNeverSignatureRequestType for error handling.
  • Refactored nativeSMASigner and singleSignerMessageSigner to utilize new signing methods.
  • Enhanced signing logic with clearer type checks and error handling.
  • Updated method signatures to improve type safety and clarity.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Copy link

vercel bot commented May 19, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
aa-sdk-ui-demo ❌ Failed (Inspect) May 22, 2025 6:51pm

Copy link

github-actions bot commented May 19, 2025

🌿 Documentation Preview

Name Status Preview Updated (UTC)
Alchemy Docs ✅ Ready 🔗 Visit Preview May 22, 2025, 6:46 PM

@github-actions github-actions bot temporarily deployed to docs-preview May 19, 2025 16:22 Inactive
@github-actions github-actions bot temporarily deployed to docs-preview May 19, 2025 18:28 Inactive
@@ -78,24 +138,18 @@ export const nativeSMASigner = (

// we apply the expected 1271 packing here since the account contract will expect it
async signMessage({ message }: { message: SignableMessage }): Promise<Hex> {
Copy link
Contributor

@Blu-J Blu-J May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 try this with a this this is assuming we have we can infer the type from references to params sent down or

async signMessage(this: SigningMethods<'eth_signTypedData_v4'>, {message}: MessageTrait) {// do things}

or do an overloaded type definition based on the type

const signMessage: Type extends 'eth_signTypedData_v4' ? (value: MessageTrait) => RT : never
const  signMessage = async ({message}) {// do things};

@github-actions github-actions bot temporarily deployed to docs-preview May 21, 2025 00:36 Inactive
@github-actions github-actions bot temporarily deployed to docs-preview May 21, 2025 17:35 Inactive
@howydev howydev marked this pull request as ready for review May 21, 2025 17:36
@github-actions github-actions bot temporarily deployed to docs-preview May 21, 2025 18:25 Inactive
@howydev howydev force-pushed the howy/split-signing-methods branch from a33fb15 to 125311e Compare May 21, 2025 18:25
@github-actions github-actions bot temporarily deployed to docs-preview May 21, 2025 18:26 Inactive
@howydev howydev force-pushed the howy/split-signing-methods branch from 125311e to 31073b5 Compare May 21, 2025 18:29
@github-actions github-actions bot temporarily deployed to docs-preview May 21, 2025 18:29 Inactive
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to update the ToSmartAccountParams or w/e it's called as well to support passing in the extensions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also looking at this, I don't think those prepare and format methods are actually exposed because of this. they need to be passed in threw params and then added to the resulting object from toSmartContractAccount

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

drat.. will relook + add a test for this

@@ -46,6 +46,21 @@ export enum DeploymentState {
DEPLOYED = "0x2",
}

export type SignatureRequest =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the type declared in @account-kit/wallet-client? I'm not sure where it's exported, but I'm pretty sure this type exists somewhere else already.

@howydev howydev force-pushed the howy/split-signing-methods branch from 31073b5 to bed7f05 Compare May 22, 2025 18:36
@howydev howydev force-pushed the howy/split-signing-methods branch from 17c88be to 3741625 Compare May 22, 2025 18:44
@github-actions github-actions bot temporarily deployed to docs-preview May 22, 2025 18:44 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants