NomicFoundation / hardhat-ignition

Hardhat Ignition is a declarative deployment system that enables you to deploy your smart contracts without navigating the mechanics of the deployment process.
https://hardhat.org/ignition
MIT License
103 stars 21 forks source link

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

Closed erhant closed 1 week ago

erhant commented 1 month ago

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

kanej commented 1 month ago

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.

erhant commented 1 month ago

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

kanej commented 1 month ago

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.

erhant commented 1 month ago

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

SebastienGllmt commented 1 month ago

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 🙂)

erhant commented 1 month ago

JSON5 makes a lot of sense actually!

kanej commented 3 weeks ago

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

erhant commented 2 weeks ago

Im taking this on, thanks for the detailed steps!

erhant commented 1 week ago

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