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

patch scaffoldConfig type error #641

Merged
merged 2 commits into from
Dec 3, 2023
Merged

Conversation

technophile-04
Copy link
Collaborator

Description

Fixes #640 for now, this removes the ability for the users to configure AddressType from scaffold.config.ts until we properly find a way to handle it (without having type error)

Maybe we can merge this for now since #640 is kind of blocker for people deploying on other networks / using multi-chain

@sverps sverps merged commit e5ed622 into main Dec 3, 2023
1 check passed
@sverps sverps deleted the patch/fix-scaffold-config-error branch December 3, 2023 21:37

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

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 👍

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.

bug: broken scaffold.config.ts for targetNetwork other than hardhat / foundry
3 participants