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.

Sign up for GitHub

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
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions account-kit/react/src/hooks/useSmartAccountClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
"use client";

import { type OptionalFields } from "@aa-sdk/core";
import type {
GetSmartAccountClientParams,
GetSmartAccountClientResult,
Expand All @@ -21,9 +21,12 @@ export type UseSmartAccountClientProps<
TAccount extends SupportedAccountTypes | undefined =
| SupportedAccountTypes
| undefined
> = GetSmartAccountClientParams<
TChain,
TAccount extends undefined ? "ModularAccountV2" : TAccount
> = OptionalFields<
GetSmartAccountClientParams<
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.

related: https://github.com/alchemyplatform/aa-sdk/pull/1328/files

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

>;

export type UseSmartAccountClientResult<
Expand All @@ -33,9 +36,7 @@ export type UseSmartAccountClientResult<

export function useSmartAccountClient<
TChain extends Chain | undefined = Chain | undefined,
TAccount extends SupportedAccountTypes | undefined =
| SupportedAccountTypes
| undefined
TAccount extends SupportedAccountTypes | undefined = "ModularAccountV2"
>(
args: UseSmartAccountClientProps<TChain, TAccount>
): UseSmartAccountClientResult<
Expand Down Expand Up @@ -66,7 +67,7 @@ export function useSmartAccountClient<
*/
export function useSmartAccountClient({
accountParams,
type,
type = "ModularAccountV2",
...clientParams
}: UseSmartAccountClientProps): UseSmartAccountClientResult {
const {
Expand Down