-
Notifications
You must be signed in to change notification settings - Fork 3
Feat/jvd/hardhat plugin #15
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
Conversation
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.
This looks promising, but the plugin should be in its own repo. It needs to be installable in a project using npm install
(or similar command, depending on package manager). Even the plugins provided by the Nomic Foundation itself have their own repos. The plugins are installed in each project. See solidity/package.json
There are several plugins installed, such as @nomicfoundation/hardhat-chai-matchers
, @nomicfoundation/hardhat-network-helpers
, etc.
Also, please provide a README to show how it should be used. It's not celar to me.
Yes we will move this to a separate repo. But even in the current version the plugin is a separate npm package that can be already published |
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.
This file (and also SpvPrecompile.yul) provide system contracts that are installed in the hardhat network. The contracts are only needed for generating new byte code when something changes. It is therefore only relevant for the developers of the plugin. The role of these contracts should probably be documented.
So, I think, the yul code should be somewhere in the hardhat plugging package and not in the solidity-example.
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.
See comment on file ChainwebChainId.yul.
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.
I need to add script for then to compile the contracts on each build.
I am also wondering about the devnet-accounts file. In case of devnet and hardhat those are builtin. So, it's a bit odd that the user has to generate that. All this accounts are in a BIP-44 wallet (and the allocations folder in the demo repo contains the details along with code). So, it feels there is no ned to hardcode that in yet another separate file. |
You have to tell hardhat about the accounts if the network is not the hardhat network. You either have to specify the mnemonic and path in the hardhat config file, or you have to specify the accounts. By specifying the accounts in a file, the developer can see the addresses and private keys, which are needed for certain actions and sometimes debugging. It may not be needed in the new plugin paradigm, but it is needed in the current setup. The developer MUST be albe to see all accounts and their private keys somewhere. |
It looks good. I updated the skipped integration tests to that they match what is in main in the solidity-example dir. I noticed there are still a lot of |
I use default hardhat accounts when chainweb.accounts is not presented. So technically you don’t have to pass accounts |
I think we should also adjust the account and PK output to look more like a hardhat node would: Account #0: 0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266 (10000 ETH) Account #1: 0x70997970C51812dc3A010C7d01b50e0d17dc79C8 (10000 ETH) Account #2: 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC (10000 ETH) That way the devs can also see which private keys go with which accounts Instead we see this for our devnet:
|
… runs properly against internal and external hardhat networks
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.
console.log inside smart contracts works great.
); | ||
} | ||
|
||
/** @type import('hardhat/config').HardhatUserConfig */ |
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.
This line can be removed since this project isn't using typescript.
"@nomicfoundation/hardhat-verify": "^2.0.12", | ||
"@nomicfoundation/ignition-core": "^0.15.9", | ||
"@openzeppelin/contracts": "^5.2.0", | ||
"@typechain/ethers-v6": "^0.5.0", |
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.
References to typechain, types, typescript, etc can be removed since typescript is not being used in this project. This was also removed from the demo solidity dir in the main branch.
Didn't mean to click on close PR. That was an accident. |
For chainweb devnet, the accounts file is 100% necessary because otherwise hardhat has no way of knowing about chainweb devnet's accounts. If this plugin can "see" the default hardhat accounts, then the accounts file isn't needed. You don't even really need the hardhat network config in that case, unless you want to assign it a different chainId than it has by default. |
Converted this PR to draft so that it doesn't accidentally get merged into main. |
3a412b9
to
9280036
Compare
Moved the plugin to its own repo https://github.com/kadena-io/hardhat-kadena-plugin |
Created a hardhat plugin setups the hardhat chains and spins up them.
refactored the solidity test into a new folder based on the plugin.