-
Notifications
You must be signed in to change notification settings - Fork 797
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
Conversation
|
||
type AddressType = typeof scaffoldConfig extends { useStrictAddressType: true } ? `0x${string}` : string; | ||
type AddressType = string; |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?)
There was a problem hiding this comment.
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! :)
There was a problem hiding this comment.
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 👍
Description
Fixes #640 for now, this removes the ability for the users to configure
AddressType
fromscaffold.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