-
Notifications
You must be signed in to change notification settings - Fork 587
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
Ported project to typescript. #65
base: master
Are you sure you want to change the base?
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.
First pass. I didn't review Dapp.tsx
nor tsconfig.json
yet.
require("@nomicfoundation/hardhat-toolbox"); | ||
import { HardhatUserConfig } from "hardhat/config"; | ||
import "@nomicfoundation/hardhat-toolbox"; | ||
import "@nomiclabs/hardhat-ethers" |
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 shouldn't be necessary if the toolbox is included. Does something stop working if you don't import it?
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.
You are right, import "@nomiclabs/hardhat-ethers"
is redundant. Everything works as expected without it.
I remember having some trouble getting the imports right, so this was likely an artifact of that.
.setAction(async ({ receiver }, { ethers }) => { | ||
if (network.name === "hardhat") { | ||
.setAction(async ({ receiver }, { ethers, config }) => { | ||
if (config.defaultNetwork === "hardhat") { |
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 don't think this is correct, the default network will almost surely be hardhat all the time.
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.
You are also correct with that one. It should have been:
task("faucet", "Sends ETH and tokens to an address")
.addPositionalParam("receiver", "The address that will receive them")
.setAction(async ({ receiver }, { ethers, network }) => {
if (network.name === "hardhat") {
// ...
}
// ...
Sorry for the inconvenience and thanks for your thorough review. I didn't know that there was a network
object available and I also couldn't find any reference implementation which used it. Next time I will read the docs more obsessively!
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've tested it :)
@fvictorio should we merge this into a If we do that, we should add a link to that branch in |
The entire project uses now typescript.
The typescript restrictions are relatively lose so less changes were required then (my) last time.
I've tested the app locally and everything works as expected.
Apart from the port to typescript, no other changes were done.