feat: flatten licenses flag #1223
Conversation
nachomazzara
force-pushed
the
feat/extend-flatten
branch
from
c08ec39
to
8098751
Compare
|
@fvictorio @alcuadrado any thoughts on why CI is not passing? |
| @@ -115,6 +117,11 @@ subtask( | |||
| flattened = `// SPDX-License-Identifier: ${license}\n\n${flattened}`; | |||
| } | |||
|
|
|||
| if (unifyABIEncoderV2) { | |||
| // Remove every line started with "pragma experimental ABIEncoderV2;" except the first one | |||
| flattened = flattened.replace(/pragma experimental ABIEncoderV2;\n/gm, (i => (m: string) => !i++ ? m : '')(0)); | |||
There was a problem hiding this comment.
This line is very hard to understand, is there a way to make it more readable?
There was a problem hiding this comment.
Ok, I just want to keep it as functional as I can. It was a time ago the last time I've used an IIFE.
Do you think that with better variables names works?
flattened = flattened.replace(/pragma experimental ABIEncoderV2;\n/gm, (index => (match: string) => !index++ ? match: '')(0));If not, alternative:
let index = 0
flattened = flattened.replace(/pragma experimental ABIEncoderV2;\n/gm, (match: string) => !index++ ? match : '');Other solutions will include maybe a complex regex or the usage of split & replace.
Any other idea?
There was a problem hiding this comment.
I just pushed a change for this to make it more like the code we tend to write. We usually avoid compact one-liners and err on the "verbose but clear" side.
| .addOptionalParam("removeLicenses", undefined, false, types.boolean) | ||
| .addOptionalParam("license", undefined, undefined, types.string) | ||
| .setAction( | ||
| async ( | ||
| { files, removeLicenses = false, license }: FlattenInput, | ||
| { files, unifyABIEncoderV2 = false, removeLicenses = false, license }: FlattenInput, |
There was a problem hiding this comment.
This option seems very ad hoc. I understand that this is what was required in the issue, but it doesn't feel right.
Maybe it could be called unifyPragmas, even if it only does this for the ABI encoder v2 one. If it happens that there's another pragma that needs this, we can add it in the future.
@alcuadrado wdyt?
There was a problem hiding this comment.
@fvictorio I have added it cause I forgot to add it when I made the PR.
From Remix, you get the following errors when you flattened contracts with ABIEncoder V2. I would like to keep that flag.
About the naming, unifyPragmas can be confused because you can still have different pragma solidity versions. Maybe unifyExperimentalPragmas works but we can't predict if we will have more experimental pragmas in the future.
|
@nachomazzara are tests passing for you locally? I'm getting 4 failures with the latest changes. |
|
@fvictorio We should be good to go! |
|
I'm closing this PR. There's a spec about how we should tackle this in this issue: #1499 |
|
Any reason why this PR was closed? There is no "competing" PR open that also fixes this, only an open issue. |
|
We decided to take a different direction which is described in #1499. We haven't executed on it yet, but mergint this PR would prevent us from doing it. |
Nowadays when trying to validate the code at Etherscan or other block explorers, and also trying to copy/paste the flattened code on Remix, you will get an error saying that you must have one license per file.
I propose two flags to extend
hardhat flatten:unifyABIEncoderV2: Remove all occurrences ofpragma experimental ABIEncoderV2except the first one.removeLicences: Remove all licenses of the files to be flattened.license: Add a unique license at the beginning of the file flattened.--license MITwill add// SPDX-License-Identifier: MIT.Usage:
npx hardhat flatten --unifyABIEncoderV2npx hardhat flatten --removeLicensesnpx hardhat flatten --license MITnpx hardhat flatten --license MIT --unifyABIEncoderV2