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

add optional chain param to viem deploy #783

Open
wants to merge 16 commits into
base: development
Choose a base branch
from

Conversation

johnpm-12
Copy link

see #782

MukulKolpe and others added 16 commits May 28, 2024 16:02
…omicFoundation#768)

Also exclude BNB Test chain from the zero fee gas fee logic calculation.

Resolves NomicFoundation#763 

---------

Co-authored-by: John Kane <[email protected]>
We need two tests, one to cover the standard HH node behaviour and one to cover
the anvil behaviour of returning a null rather than a boolean.
We are attempting to use `any` and testing casting less.

remove superfluous type cast
…-create2

Fix: handle anvil `hardhat_setBalance` response for create2
…rcular-imports

Handle circular imports in verify loop
The Solidity grammar suggests that the first character of an identifier must be
a letter.

It is arguable therefore, that we should do that for the args list, but I think
this increment in precision is enough.

See https://docs.soliditylang.org/en/v0.8.26/grammar.html#a4.SolidityLexer.Identifier
…oad-brackets

Adjust regex to handle square brackets
@zoeyTM
Copy link
Contributor

zoeyTM commented Jun 17, 2024

I actually don't think this is needed at all. We have access to the chain ID, either by querying the network or via the custom one configured in hardhat config. All we need to do is pass the existing chain ID into the viem functions as well. No need for a new parameter. @kanej

@alcuadrado
Copy link
Member

Agreed with @zoeyTM.

Can you explain the motivation behind a new param? Is it because of a type-level limitation?

@zoeyTM zoeyTM force-pushed the development branch 2 times, most recently from 6f5ec1c to 1675490 Compare June 17, 2024 16:50
@johnpm-12
Copy link
Author

johnpm-12 commented Jun 18, 2024

I actually don't think this is needed at all. We have access to the chain ID, either by querying the network or via the custom one configured in hardhat config. All we need to do is pass the existing chain ID into the viem functions as well. No need for a new parameter. @kanej

I originally considered defining the custom chain inside the deploy function automatically if the chain id doesn't match a supported viem chain. But you have to make assumptions about native currency name, symbol, and decimals. They're required params when defining a viem chain.

https://viem.sh/docs/chains/introduction#custom-chains

EDIT: also I figured it should support functionality similar to the other viem hardhat functions (getPublicClient, getWalletClients, etc). They solve this problem with an optional chain parameter

@zoeyTM
Copy link
Contributor

zoeyTM commented Jun 19, 2024

I actually don't think this is needed at all. We have access to the chain ID, either by querying the network or via the custom one configured in hardhat config. All we need to do is pass the existing chain ID into the viem functions as well. No need for a new parameter. @kanej

I originally considered defining the custom chain inside the deploy function automatically if the chain id doesn't match a supported viem chain. But you have to make assumptions about native currency name, symbol, and decimals. They're required params when defining a viem chain.

https://viem.sh/docs/chains/introduction#custom-chains

EDIT: also I figured it should support functionality similar to the other viem hardhat functions (getPublicClient, getWalletClients, etc). They solve this problem with an optional chain parameter

I think this functionality would be better served via extending hardhat config, adding the necessary fields to custom chains there, then pulling the info from hre.config inside the ignition helper.

@johnpm-12
Copy link
Author

I actually don't think this is needed at all. We have access to the chain ID, either by querying the network or via the custom one configured in hardhat config. All we need to do is pass the existing chain ID into the viem functions as well. No need for a new parameter. @kanej

I originally considered defining the custom chain inside the deploy function automatically if the chain id doesn't match a supported viem chain. But you have to make assumptions about native currency name, symbol, and decimals. They're required params when defining a viem chain.
https://viem.sh/docs/chains/introduction#custom-chains
EDIT: also I figured it should support functionality similar to the other viem hardhat functions (getPublicClient, getWalletClients, etc). They solve this problem with an optional chain parameter

I think this functionality would be better served via extending hardhat config, adding the necessary fields to custom chains there, then pulling the info from hre.config inside the ignition helper.

And generate the custom chain internally if it's defined in the hardhat config?
What if a custom chain is not fully defined in the hardhat config? Throw an error?
I assume getpublicClient, getWalletClients, and the other hardhat viem functions would need to be updated to do this, too?

I can do it this way, I just want to make sure I understand how ya'll want it done before I get started

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants