pcaversaccio / xdeployer

Hardhat plugin to deploy your smart contracts across multiple EVM chains with the same deterministic address.
https://www.npmjs.com/package/xdeployer
MIT License
440 stars 42 forks source link

💥 Add `chainId` to User Configuration #284

Closed Confucian-e closed 2 months ago

Confucian-e commented 2 months ago

Describe the desired feature:

Now we have to set network name in networks field. It takes time to find the correct name that matchs. Maybe you can consider supporting chainId as an optional feature?

Code example that solves the feature:

  xdeploy: {
    contract: "Counter",
    // constructorArgsPath: "", // optional; default value is `undefined`
    salt: "test",
    signer: PrivateKey,
    networks: [1, 10, 56],
    rpcUrls: [Ethereum_RPC_URL, Optimism_RPC_URL, BSC_RPC_URL],
    gasLimit: 1_500_000, // optional; default value is `1.5e6`
  },
pcaversaccio commented 2 months ago

Hey, thanks for opening this feature request. The issue with using chainIds is that they aren't inherently clear or human-readable in terms of identifying which network is being used. Additionally, some people prefer to use hex notation for the chainId, adding another layer of complexity to parse. I'm hesitant to add chainIds because I want to encourage developers to be explicit and deliberate in their choices. Typically, the network name is easier and quicker to identify, while the chainId often takes longer to find (and validate) and can sometimes be confused with the networkId, which may differ across various chains. Keeping things simple is better in this case.

Confucian-e commented 2 months ago

Hey, thanks for opening this feature request. The issue with using chainIds is that they aren't inherently clear or human-readable in terms of identifying which network is being used. Additionally, some people prefer to use hex notation for the chainId, adding another layer of complexity to parse. I'm hesitant to add chainIds because I want to encourage developers to be explicit and deliberate in their choices. Typically, the network name is easier and quicker to identify, while the chainId often takes longer to find (and validate) and can sometimes be confused with the networkId, which may differ across various chains. Keeping things simple is better in this case.

I understand your idea. So, could you add a command to the xdeploy task that lists all currently available networks? For example, something like npx hardhat xdeploy --list. Additionally, display their corresponding chainIds so that users can search for the corresponding names based on the chainIds. Of course, this is an optional feature, but I believe it is necessary to include a command that lists all network names.

Confucian-e commented 2 months ago

Hey, thanks for opening this feature request. The issue with using chainIds is that they aren't inherently clear or human-readable in terms of identifying which network is being used. Additionally, some people prefer to use hex notation for the chainId, adding another layer of complexity to parse. I'm hesitant to add chainIds because I want to encourage developers to be explicit and deliberate in their choices. Typically, the network name is easier and quicker to identify, while the chainId often takes longer to find (and validate) and can sometimes be confused with the networkId, which may differ across various chains. Keeping things simple is better in this case.

I understand your idea. So, could you add a command to the xdeploy task that lists all currently available networks? For example, something like npx hardhat xdeploy --list. Additionally, display their corresponding chainIds so that users can search for the corresponding names based on the chainIds. Of course, this is an optional feature, but I believe it is necessary to include a command that lists all network names.

I made a PR to add a command to list networks. You can check it https://github.com/pcaversaccio/xdeployer/pull/285

pcaversaccio commented 2 months ago

Thx for the PR - I enriched it with the chain IDs now:

image

Are you ok with this solution?

Confucian-e commented 2 months ago

That's awesome. Just merge and publish new npm pacakge.

pcaversaccio commented 2 months ago

I will merge it probably tomorrow as I need to revalidate my code first :)

pcaversaccio commented 2 months ago

Published the new version: https://github.com/pcaversaccio/xdeployer/releases/tag/v3.1.0.