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

Ported project to typescript. #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Throvn
Copy link

@Throvn Throvn commented Jul 8, 2022

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.

Copy link
Member

@fvictorio fvictorio left a 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"
Copy link
Member

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?

Copy link
Author

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") {
Copy link
Member

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.

Copy link
Author

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested it :)

@alcuadrado
Copy link
Member

@fvictorio should we merge this into a typescript branch?

If we do that, we should add a link to that branch in master's README

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

Successfully merging this pull request may close these issues.

3 participants