-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Devise new experimental @truffle/from-hardhat package for compatibility translation #5420
Conversation
e0bc557
to
bcedc89
Compare
"devDependencies": { | ||
"@types/find-up": "^2.1.0", | ||
"@types/node": "^18.6.5", | ||
"hardhat": "^2.10.1", |
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.
@cds-amal @fainashalts FYI I realized that we can benefit from Hardhat's own types by keeping this as a devDependency safely... if we just don't actually export anything with these types in the signature. This is the approach I'm taking here.
Just letting you know because this breaks the assumptions of our earlier 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.
how does this compare to making it an optional peerDepenency?
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.
how does this compare to making it an optional peerDepenency?
Good question! Firstly, it's worth noting that package.json
's peerDependenciesMeta
field (which is how you mark peerDeps as optional) was added in NPM v7, and we need to support v6 still. But this is a minor quibble.
The main concern here, IMO, is the question "what does Truffle's Node.js-land need from the "hardhat"
NPM package?"
- Types means we need at least a
devDep
- We need more than
devDep
if we want to have a shared Node.js runtime between Truffle and Hardhat... as in, if we want to use Hardhat as a library
I looked at using HH as a library, but, like Truffle, it's really not intended for use via straight require()
(or import
). I got it working in #5410... so this approach is viable albeit awkward. But think about what we're saying when Truffle does require("hardhat")
... like, "here you go, Node! have another, different one of me!". Even without the risk of cross-clobbering each other's globals / other runtime modification shenanigans, the memory footprint and whatnot... oy!
So my conclusion was basically... screw peerDependencies
; HH is prescriptive: you use it via npx hardhat
. Truffle can use HH via npx hardhat
. Keep the playgrounds separate :)
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.
Probably worth following up that there is a counter-argument that we could just use peerDependencies
for the $(npm bin)
or whatever. But still... I think there's value in treating HH as an executable... like, you're not going to try to shove Docker into your package.json
- you just shell out to docker
and error nicely if it's not there.
Oh, and another follow-up argument might be: we could use peerDependencies
to leverage npm/yarn's semver powers to avoid manual version compatibility checks ourselves at runtime... and yeah, we could, but I don't think it's worth the added surface area of complexity around managing this peerDependency... like, who gets the actual dependency? Truffle? Like, oof.
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.
Ah that makes sense, and feel free to ignore my repeat question in the review below.
I was just thinking that it'd be very nice if there was some way to import just the types from the hardhat package without needing to add all of their transitive dependencies into our node_modules
tree and our yarn.lock
(even if it is just for dev purposes).
If our usage of their types is minimal, perhaps it'd be worth extracting only the types that we need into a local type declaration? Though I could also easily argue against that approach due to the potential effort involved in doing that, along with the increased maintenance burden that will be required.
Sure would be nice if there was a code-gen utility for that sort of thing, though!
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.
Hm, I see your concern. Not sure copy+pasting the types is acceptable? Sounds like follow-on work? Or maybe we could get HH to publish types separately?
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.
Yeah, I think if we make changes to this approach, it's definitely something that's a future concern.
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.
Ah, I resolved this, but realized that neither of the two people who you pinged at the start have replied, so I've unresolved 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.
Otherwise please consider my part in this resolved.
69a02ce
to
6e9b1ff
Compare
... for compatibility translation
cc @alcuadrado this @truffle/from-hardhat PR is now ready for review - I believe I handled the concerns you stated in #5410, but would love if you could take another look! Thank you! |
options?: EnvironmentOptions | ||
): Promise<TruffleConfig> => { | ||
const hardhatConfig = (await askHardhatConsole( | ||
`hre.config`, |
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.
Worth noting about this approach: this only works because hre.config
is serializable.
I am so grateful and so jealous that they made this serializable :)
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.
Part of the struggle of forging our own path in the early days of the Ethereum ecosystem, I suppose. Also the node ecosystem and what is considered best practice has advanced just a touch since 2015!
Like, I think at the time that the original truffle-config.js
was designed executable config was the hotness, as it was (and still is) super flexible. Now it's somewhat frowned upon for multi-modal projects like ours due this exact concern.
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.
There's trade-offs... like, I also had the thought about how easy it is to read private keys from memory. Our approach clamps that down, since we only store mnemonics inside closures.
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.
Requested changes are mostly a bunch of small things, with a much larger pile of questions.
Also I do think it would be nice if this had tests, but as discussed on our 1:1, I'd agree that writing fast tests for this could take some nontrivial test engineering effort, so I'd be happy enough to agree that tests could be done in another PR. That said, if you're likely going to put this down for a while before returning to it, perhaps we should capture a ticket to that effect.
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.
LGTM!
Thanks @benjamincburns! Will merge when green. |
(spun out from #5410... please see #5434 for the work to actually provide
truffle debug
support)Overview
This PR introduces a new, experimental package @truffle/from-hardhat to provide the following interfaces:
expectHardhat()
, which returns successfully iff the working directory is part of a Hardhat project, and iff the installed Hardhat version is supported (^2.10.1
, current as of this PR). Exceptions are thrown to distinguish these failure modes. (This function is intended for use inside atry
/catch
)prepareCompilations()
that translates Hardhat "BuildInfo" into our native @truffle/compile-common "Compilation"prepareConfig()
which minimally reads the Hardhat config and converts it into a @truffle/config. Basically this only contains plainurl
networks. 🔍 See documented caveats:truffle/packages/from-hardhat/src/api.ts
Lines 65 to 94 in e9da6d9
Intended use
This PR intends for this package to be used by specific read-only Truffle commands, primarily
truffle debug
. At this time, this PR does not seek to provide any kind of support fortruffle compile
,migrate
,test
, etc. This package does not likely even provide enough information for most of other read-only commands, e.g.truffle networks
, because no attempt is made to encode a means by which to convert deployment information into Truffle's artifacts.🤔 Why so limited? Because even
truffle debug
is heckin' useful, and I posit that we should do this despite its being scrappy and experimental because it's 🔥 🔥 🔥Approach
This PR seeks to avoid intermixing Truffle's and Hardhat's Node.js runtimes. This new package includes Hardhat as a
devDependency
for its types, and keeps this dev- guarantee by not exposing any reference to these types. This ensures that we're not at risk of users' Truffle installations inadvertently including a full install of Hardhat as well.How? This shells out to
npx hardhat
! Information about the Hardhat project is requested vianpx hardhat console
and passed along stdio via minified JSON. This ensures that Truffle's [likely polluted] Node.js runtime does not cause any conflicts with Hardhat's own [likely polluted] Node.js runtime.These are necessary because neither Truffle nor Hardhat were really built to be library-first, and it seems prudent to treat Hardhat like a regular system executable on the PATH. (Well,
npx
is the executable on the PATH, but Hardhat specifically only supports running vianpx
)