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

patch scaffoldConfig type error #641

Merged
merged 2 commits into from
Dec 3, 2023
Merged

patch scaffoldConfig type error #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93bca27
remove import of scaffold.config from abi.d.ts
technophile-04 Dec 3, 2023
e45844c
remove useStrictAddressType option from scaffol.config for now
technophile-04 Dec 3, 2023
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
7 changes: 0 additions & 7 deletions packages/nextjs/scaffold.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ export type ScaffoldConfig = {
walletConnectProjectId: string;
onlyLocalBurnerWallet: boolean;
walletAutoConnect: boolean;
useStrictAddressType?: boolean;
};

const scaffoldConfig = {
Expand Down Expand Up @@ -39,12 +38,6 @@ const scaffoldConfig = {
* 2. If user is not connected to any wallet: On reload, connect to burner wallet if burnerWallet.enabled is true && burnerWallet.onlyLocal is false
*/
walletAutoConnect: true,

/**
* Strict address types makes viem/wagmi use `0x${string}` instead of plain string for addresses
* Leave this undefined or set to the default value if you're new to TypeScript
*/
useStrictAddressType: false,
} as const satisfies ScaffoldConfig;

export default scaffoldConfig;
3 changes: 1 addition & 2 deletions packages/nextjs/types/abitype/abi.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import "abitype";
import scaffoldConfig from "~~/scaffold.config";

type AddressType = typeof scaffoldConfig extends { useStrictAddressType: true } ? `0x${string}` : string;
type AddressType = string;
Copy link
Member

Choose a reason for hiding this comment

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

If you have 0x${string} here, does it work?

Just saying because if the issue is just the useStrictAddressType flag, we could also have the flag right here (with a nice comment)

For me useStrictAddressType feels a bit too specific for the scaffold config file (where we are setting API keys, target networks, etc)

Just something to think about!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, fair point about it being probably a bit too specific to be a main config in scaffold.config.

Additional exploration lead me to the probable root cause: chains other than local chains seem to have defined a couple of contracts that have an address field typed by abitype. This creates a circular reference (abi.d.ts override -> scaffold.config.ts -> chains -> address -> abitype), which doesn't work.

So at first glance, it seems that there's no simple fix anyway that would allow us to keep it inside scaffold.config. In any case, meddling with this addess type is probably for advanced users. Maybe we can just document overriding the address type in the readme and explain anyone that wants more strict types to just remove our abi.d.ts override (I tested it and it seems to do the trick).

@technophile-04 @carletex what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you have 0x${string} here, does it work?

Do you mean to say we should have 0x{string} as default here?

chains other than local chains seem to have defined a couple of contracts that have an address field typed by abitype. This creates a circular reference (abi.d.ts override -> scaffold.config.ts -> chains -> address -> abitype), which doesn't work.

Ohhhhhh yeah, this makes sense !!!! Tysm for great explanation 🙌 yup there are indeed contracts like multicall3, ensRegistry in most of chains with address.

Just saying because if the issue is just the useStrictAddressType flag, we could also have the flag right here (with a nice comment)

For me useStrictAddressType feels a bit too specific for the scaffold config file (where we are setting API keys, target networks, etc)

So at first glance, it seems that there's no simple fix anyway that would allow us to keep it inside scaffold.config. In any case, meddling with this addess type is probably for advanced users.

Yup yup 💯 agree with you guys !! I think adding few documentation lines in abi.d.ts and maybe mentioning about this abi.d.ts in QuickStart -> Environment section is minimal and clean approach. Something like how wagmi beta docs have this typescript tip toggle in their getting started section.

Will create an issue for this in SE-2 docs repo 🙌

explain anyone that wants more strict types to just remove our abi.d.ts override (I tested it and it seems to do the trick).

Instead of removing abi.d.ts what if we tell them to just update type AddressType = 0x{string}`` instead of string if they want stricter type, I just tried switching it and everything works nicely for me 🙌 (Maybe Carlos was talking about this thing in first comment ?)

Copy link
Member

Choose a reason for hiding this comment

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

(Maybe Carlos was talking about this thing in first comment ?)

Yes! I also think the string default is the way to go (beginner friendly). Just wanted to know if the issue was the flag in scaffold.config or using 0x{string} itself.

I agree with you both, we can just have something in the README/Se2-docs

Thanks! :)

Copy link
Collaborator

@sverps sverps Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of removing abi.d.ts what if we tell them to just update type AddressType = 0x{string}`` instead of string if they want stricter type, I just tried switching it and everything works nicely for me 🙌 (Maybe Carlos was talking about this thing in first comment ?)

Yeah all good to me, cause both removing the file or setting the type there to `0x${string}` should have the same result. So in summary, no further code changes necessary, just some info to add in the readme 👍


declare module "viem/node_modules/abitype" {
export interface Config {
Expand Down