hardhat-ignition icon indicating copy to clipboard operation
hardhat-ignition copied to clipboard

feat: Allow `... ether` to be parsed in parameters

Open erhant opened this issue 1 year ago • 6 comments

Describe the feature

Since we already have a custom deserializer for parameters (https://github.com/NomicFoundation/hardhat-ignition/issues/663 to parse bigint) i think it makes most sense to also parse w.r.t parseEther of ethers, so that instead of writing "1000000000000000000n" as parameter which is definitely not human-readable, I could type 1.0 ether, based on .endsWith keyword it can directly parse the value.

In this way, 1.0 gwei could also be parsed and such.

P.S. would love to work on this!

Search terms

parameters

erhant avatar Jul 31 '24 14:07 erhant

I suspect we would hesitate before increasing the complexity of the deserialization process. Currently it is JSON.parse with this one additional magic rule for bigints, any additional rules would have to justify the additional complexity.

If this is people see as useful, can I ask you upvote and leave a description of the use case it would help in.

kanej avatar Aug 01 '24 09:08 kanej

thinking of less invasive methods, I was wondering what if we instead allow updating a parameter at runtime, e.g.

const param = m.getParameter("param-name");
m.setParameter("param-name", param ** 18n); // if param was "10", now its equal to "10 ether"

// allow function argument as well
m.setParameter("param-name", (param) => param ** 18n);

EDIT 1: This way, the parsing logic is fully left to the user's hand and they can do whatever they want with it, not just for currency as well but other custom logic such as deriving one parameter from another at runtime

erhant avatar Aug 01 '24 09:08 erhant

Hey @erhant, firstly thanks for the suggestions, we have been talking over the approach to take on this internally.

Complexity in the deserializer we try and avoid, but support for arbitrary anonymous functions in the module API makes serializing and deserializing the Module data structures hard - and that is a capability we want to keep.

Some complexity in the deserializer seems like a better tradeoff here, so in effect your first suggested approach.

Let me put together some notes on how to approach adding to the deserializer, I will post them here and we discuss it further if your interested.

kanej avatar Aug 07 '24 09:08 kanej

hey, thanks for letting me know 🙏🏻

Let me put together some notes on how to approach adding to the deserializer, I will post them here and we discuss it further if your interested.

I would love that, looking forward!

EDIT: formatting

erhant avatar Aug 07 '24 11:08 erhant

I think the better solution to this problem is to instead make the parameters.json using JSON5 instead of JSON, as JSON5 is very well supported as a standard, is trivial to turn to native JSON later if needed, and it also allows comments

That is to say, I think if we could something like this

"MyModule": {
    "endowment": "1000000000000000000n" // 1 ETH in wei
  }

That would be great and a lot more flexible than needing new units like ether for every single use-case (hey look, even Github supports json5 markdown blocks 🙂)

SebastienGllmt avatar Aug 11 '24 00:08 SebastienGllmt

JSON5 makes a lot of sense actually!

erhant avatar Aug 12 '24 06:08 erhant

The approach then would be to use json5 when parsing the parameters.json file. We would retain the replacer function that converts strings of the "1000n" format to native javascript BigInts. We won't attempt to leverage JSON5's arbitrary-precision integers to support BigInts. This will allow users to comment the parameters file to provide context. We should also check we support the .json5 extension for parameter files.

TODO

  • [ ] Add json5 as a dependency: https://www.npmjs.com/package/json5
  • [ ] Update the filename checks to support .json5 as well as .json
  • [ ] Update the parsing code here, keeping the bigint reviver: https://github.com/NomicFoundation/hardhat-ignition/blob/c4278b68450535781d734ea9fe02a4d654df7a48/packages/hardhat-plugin/src/index.ts#L675
  • [ ] Add an integration test with an example project with json5 based parameters file

kanej avatar Aug 21 '24 14:08 kanej

Im taking this on, thanks for the detailed steps!

erhant avatar Aug 24 '24 19:08 erhant

Sorry I barely had time until now, busy week! PR open 🙏🏻

erhant avatar Sep 01 '24 13:09 erhant