truffle

truffle copied to clipboard

(spun out from #5410)

This PR adds a new const { Shims } = require("@truffle/compile-solidity"); to expose functions for converting bytecode in solc's format into bytecode in our format. These functions are already in use internally when processing solc output in compile-solidity itself, but I propose that this behavior is more widely useful across Truffle, and this warrants their becoming a proper export.

The usefulness of these functions stems from the view that solc's output format is used across the ecosystem, and many other tools expose data in solc's format. In prototyping #5410, I observed the need for this bytecode conversion because Hardhat stores their bytecode in the native solc format. Continuing from #5410, my hope is to use these new exports in perhaps a "@truffle/from-hardhat" package.

Considerations:

• In separating out the changes for this PR, I had the question in my mind of where to put these shims? I think @truffle/compile-solidity is the obvious answer, but I initially started adding these types to @truffle/compile-common as a Common.Shims.FromSolc sub-namespace in the existing Common.Shims. But clearly this is compile-solidity related and the functions should remain there. (I wasn't sure about types at first, since compile-solidity is semi-JS, but the exports seem fine! tsc is smart!)

• There's an argument that this sort of thing is putting the cart before the horse, so to speak: don't add the exports until something else needs them! Please advise during review if that approach has more merit... happy to turn this draft and/or get it against a feature branch. It just seemed innocuous enough to me to hit develop directly, cause, why not?

Addendum: sorry about the prettier changes. All I did was move two functions 😀

There's an argument that this sort of thing is putting the cart before the horse, so to speak: don't add the exports until something else needs them! Please advise during review if that approach has more merit.

I think that's essentially my initial feedback (I'll still read through the code, however). Or put slightly differently, I think this code would be easier to review if I could see an example of this shim in use, as otherwise I'm evaluating its fitness for a purpose that I don't fully understand.

In separating out the changes for this PR, I had the question in my mind of where to put these shims?

I find that code is always more testable and otherwise easier to reason about when the direct control "ah, now which impl do I need?" logic is kept separated from the consumption of said implementation. I think that may possibly be what you were struggling with here, in the absence of consumer code to design around.

I think that may possibly be what you were struggling with here, in the absence of consumer code to design around.

Note that this code already has a consumer - compile-solidity already uses it. I was originally thinking of putting it into compile-common, since I foresee a third package that will use it, and compile-common is lower in the dependency tree. And because compile-common already has a Common.Shims module. But compile-solidity is where our solc integration lives entirely, and thus I concluded against breaking that.

Now that I've reviewed it - I do still think it would be easier to evaluate this change in the spirit of its intent if there was an example of an external consumer.

You got it! Please hold... 😛

That said, I think if you just dropped the additional exports for now, I'd be happy to see this merged without any additional changes, as if nothing else it improves the cohesiveness of the internal modules in the compile-solidity package.

~Hm... I guess I'd get more commit credit that way... Alright fine, but you know the point was that I wanted these exports :)~

Update: Nope I'm too lazy to rewrite the PR description. I'll just get the consumer side up soon :)

~If you wanna drop the exports we can just merge this now (or tomorrow or whatever) and you can do the consumer side (and exports) in a second PR?~

Edit: ah, ignore me - I see what you're saying. You'd rather avoid rewriting the PR description so you'll do it here. Gotcha.

I'll have to get back to this on my-Thursday @benjamincburns, but check out the draft I just threw up for a new @truffle/from-hardhat package in #5420. See in particular the code around here, where these shims get used externally

After having a look at how you intend to use this, I think it's fine to merge for now as-is. I think I was being a bit too sensitive to the risk of introducing a minor version bump that might need a breaking change in the future, especially since I doubt this package is widely used outside of Truffle.

After having a look at how you intend to use this, I think it's fine to merge for now as-is. I think I was being a bit too sensitive to the risk of introducing a minor version bump that might need a breaking change in the future, especially since I doubt this package is widely used outside of Truffle.

Thanks! Oh yeah, definitely not used outside of Truffle :)