Skip to content
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

fix: make type really optional, add defaults for return type #1338

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

howydev
Copy link
Collaborator

@howydev howydev commented Feb 9, 2025

Pull Request Checklist


PR-Codex overview

This PR modifies the useSmartAccountClient hook in the account-kit/react/src/hooks/useSmartAccountClient.ts file to improve type handling and default values for account types.

Detailed summary

  • Added OptionalFields to GetSmartAccountClientParams.
  • Changed the default type of TAccount to "ModularAccountV2".
  • Updated the type parameter in the function signature to default to "ModularAccountV2".

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

Copy link

vercel bot commented Feb 9, 2025

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

Name Status Preview Comments Updated (UTC)
aa-sdk-site ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 9, 2025 9:05pm
aa-sdk-ui-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 9, 2025 9:05pm

TChain,
TAccount extends undefined ? "ModularAccountV2" : TAccount
>,
"type"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should "type" here be mode? There's actually another inner parameter to a client constructor called type that's just an inner string field, which makes any uses of type after the switch to mode still compile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

related: if GetSmartAccountClientParams already has mode as optional from https://github.com/alchemyplatform/aa-sdk/pull/1335/files, then I think we can skip using OptionalFields<...> here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we're doing both optional - the client level type defaults to MAv2 and the account level mode defaults to default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with this, useSmartAccountClient({}) becomes valid syntax

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be overriding type? I thought we needed the client type to be alchemy for the alchemy gas estimator logic to get correctly used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh nevermind, I think that's for .type within the transport passed to the client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that seems to be transport.config.type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

im so glad we changed the account type to mode lol

Copy link
Collaborator

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Ah, I think I just understood the relationship between the default in the generic and the default-initialized param - this looks good!

@howydev howydev merged commit 5afc25f into adam/7702-core Feb 10, 2025
7 checks passed
@howydev howydev deleted the howy/sdk-fixes-2 branch February 10, 2025 20:22
adamegyed added a commit that referenced this pull request Feb 10, 2025
…-kit/smart-contracts (#1287)

* feat: sma 7702

* feat: adds 7702 support to alchemy signer (#1269)

* feat: adds initial impl of signAuthorization

* docs: adds signAuthorization documentation

* fix: rename var

* feat: 7702 progress

* feat: update viem and debug 7702 auth

* feat: add deterministically deployed demo nft

* fix: don't re-sign delegations

* feat: update 7702 middleware to higher order function

* chore: merge fixes

* fix: remove unnecessary console log

* fix: correctly await client actions in infra test

* fix: correctly encode yParity in zero case

* feat: correctly set validation entity in nonce and separate encode actions

* feat: consolidate MAv2Base

* feat: add unified ma v2 client (#1309)

* feat: add unified ma v2 client

* chore: update docs

* chore: make account source unique

* feat: add ma v2 account to useSmartAccountClient hook (#1314)

* feat: add unified ma v2 client

* chore: make account source unique

* feat: add ma v2 account to use smart contract client hook

* chore: remove different type names

* chore: rename mav2 to ModularAccountV2

* fix: review fix, add common type

* fix: typo

* fix: remove await

* fix: narrow the type instead of doing non null assertion

* chore: rebaseme

* fix: client type and middleware inclusion

* docs: update docs with twoslash

* feat: update to new json format for auth

* feat: add defaults for useSmartAccountClient to ma v2 (#1328)

* fix: fix MAv2 React Hook client creation for 7702 (#1329)

* fix: rename MAv2 type to mode, add 7702 middleware for react hook

* fix: don't switch MintCard to MAv2 yet

* fix: reconnect 7702 account after page refresh

* chore: remove comment

* docs: regen docs

* fix: fix docs

* fix: remove unnecessary optional chaining

* feat: update eip 7702 auth format

* fix: make mode optional in useSmartAccountClient (#1335)

* chore: move optional mode into account params (#1339)

* chore: move optional mode into account params

* chore: removed unused var

* fix: make type really optional, add defaults for return type (#1338)

* refactor: assert exhaustive account type handling in getSmartAccountClient

---------

Co-authored-by: Linna <[email protected]>
Co-authored-by: howy <[email protected]>
Co-authored-by: jakehobbs <[email protected]>
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.

2 participants