feat: Allow renaming exported ABIs

[phated]

This allows users to rename their exported ABIs without any additional task hooks or filesystem reads. They can specify a rename function that receives the fully qualified name and returns a new name.

[ItsNickBarry]

Specific use case for this? Would it be more useful to pass the entire sourceName + contractName to the rename function?

[ItsNickBarry]

Never mind the second question, I misread the code.

[phated]

Specific use case for this?

Alone, this doesn't do much. But I plan to submit a couple more PRs:

1. The ability to specify an array of configuration for separate export logic
2. The ability to filter an ABI while exporting. This is necessary to export ABIs that are compatible with The Graph (which fails on non-view or multi-dimensional functions in the ABI - ref multi dimensional tuples tuple[][] graphprotocol/graph-tooling#588)

[phated]

I'm trying to replace a bunch of custom logic in Dark Forest with this plugin, as we migrate to the Diamond pattern: https://github.com/darkforest-eth/eth/blob/master/tasks/compile.ts#L14-L88

[phated]

@ItsNickBarry what do you think of this new approach? I'm a fan! I've also marked this as breaking since the flat option was removed.

[ItsNickBarry]

I still don't see much use for the rename option (except for the flat setting), so I'd like to better understand the next steps you have planned before merging. My primary concern is that the configuration schema might get too complex.

I also think I might want to keep the flat option - I'm working towards a sort of unification of my plugins, and flat has a parallel here: ItsNickBarry/hardhat-contract-sizer#12

So right now I'm thinking that rename might be better implemented as an override of a save-abi-to-file subtask (yet to be added, but could be).

As for the subgraph issue, it's very disappointing to see a multi-billion-dollar project leave something like that unaddressed for so long. No excuse, really. I am in favor of providing a workaround here, but it should be kept on the periphery and its implementation should not affect core functionality of the plugin.

[phated]

Hm, so you want me to back out these changes? As for overriding tasks, I'm trending towards being very much against them. What happens with task overrides is a cascading effect where multiple other projects do it and then you have to import them in a very specific order. This is a design flaw in hardhat and stops them from every supporting ESM without a rework.

Ideally, the plugins will provide common functionality as configuration and then anything exceptionally specialized will use an override. I don't believe Dark Forest is doing anything too far out of the ordinary (I've seen other projects doing much the same things).

[phated]

As for your contract-sizer plugin, I don't think the flat option does quite the same thing because you aren't writing files to the filesystem, so conflicts will just be confusing, not destructive.

[phated]

How would you feel if I implement the rest of my changes on top of this PR so you can see my plans for this rename option?

[ItsNickBarry]

Hm, so you want me to back out these changes? As for overriding tasks, I'm trending towards being very much against them. What happens with task overrides is a cascading effect where multiple other projects do it and then you have to import them in a very specific order. This is a design flaw in hardhat and stops them from every supporting ESM without a rework.

Not saying that these changes are not acceptable, just that I need more time and information to make an informed decision. Don't want to rush it, particularly with respect to breaking changes (also, it's been a long couple of weeks, so I'm not necessarily thinking as clearly as I would like).

I do see your point about subtasks, and have encountered issues with import ordering in the past. However, I think that in 95% of cases, that only applies to built-in Hardhat subtasks. If hardhat-abi-exporter exposes a subtask, only end-user Hardhat projects are expected to ever override it, so the task "inheritance" tree remains flat. Still seems like a valid concern, because we cannot predict user behavior, so I will weigh it against the complexity of your proposed extension to the configuration.

How would you feel if I implement the rest of my changes on top of this PR so you can see my plans for this rename option?

Yes, I'd like to see either an explanation or an implementation, whichever you feel is easier.

As for your contract-sizer plugin, I don't think the flat option does quite the same thing because you aren't writing files to the filesystem, so conflicts will just be confusing, not destructive.

Both plugins should probably throw an error if the final output includes name conflicts (I'll look into it). Ultimately, confusion is in itself destructive. So if flat means "flatten file path before output" then I still think there's a strong parallel. But don't worry about this either way, it's not a primary concern here.

[phated]

I can try to describe it!

I'm in the process of migrating the Dark Forest contracts to the Diamond pattern, and in that process we combine all our Facet ABIs into a single ABI named DarkForest.json. We use a plugin I wrote for this: https://github.com/projectsophon/hardhat-diamond-abi

This is simple enough, with both plugins, we can write a hardhat.config.ts like this:

const config = {
  solidity: '0.8.10',
  diamondAbi: {
    // This plugin will combine all ABIs from any Smart Contract with `Facet` in the name or path and output it as `DarkForest.json`
    name: 'DarkForest',
    include: ['Facet'],
  },
  abiExporter: {
    // This plugin will copy the ABI from the DarkForest artifact into our `@darkforest_eth/contracts` package as `abis/DarkForest.json`
    path: path.join(packageDirs['@darkforest_eth/contracts'], 'abis'),
    runOnCompile: true,
    // We don't want additional directories created, so we explicitly set the `flat` option to `true`
    flat: true,
    // We **only** want to copy the DarkForest ABI (which is the Diamond ABI we generate) to this folder, so we limit the matched files with the `only` option
    only: [':DarkForest$'],
  },
};

export default config;

We want this entire ABI available for the game's dApp, but we also need to generate and export a version of that same ABI that is valid for the Dark Forest Subgraph (which is heavily used by the playerbase). So my goal is to allow separate "abi exporters", like such:

const config = {
  solidity: '0.8.10',
  diamondAbi: {
    // This plugin will combine all ABIs from any Smart Contract with `Facet` in the name or path and output it as `DarkForest.json`
    name: 'DarkForest',
    include: ['Facet'],
  },
  // This plugin config will copy the ABI from the DarkForest artifact into our `@darkforest_eth/contracts` package
  abiExporter: [
    // Generates the `abis/DarkForest.json` file for use in the game client
    {
      path: path.join(packageDirs['@darkforest_eth/contracts'], 'abis'),
      runOnCompile: true,
      // We are only generating one file so set the name to "DarkForest" explicitly
      rename: () => 'DarkForest",
      // We **only** want to copy the DarkForest ABI (which is the Diamond ABI we generate) to this folder, so we limit the matched files with the `only` option
      only: [':DarkForest$'],
    },
    // Generates the `abis/DarkForest_stripped.json` file 
    {
      path: path.join(packageDirs['@darkforest_eth/contracts'], 'abis'),
      runOnCompile: true,
      // We are only generating one file so set the name to "DarkForest_stripped" explicitly
      rename: () => "DarkForest_stripped"
      only: [':DarkForest$'],
      filter: (abi) => subgraphAbiFilter(abi),
    },
  ]
};

export default config;

This will allow us to turn the same ABI into multiple exported ABIs and support both the game client and the subgraph.

So far we've just been doing this sort of logic by hooking the compile and generating the artifacts ourselves; however, integrating the Diamond ABI with TypeChain has run into the task hook ordering problem we were discussing. While trying to solve those issues, I thought it'd be helpful to combine efforts with already-released community plugins (Dark Forest has waaaaaay too much bespoke code).

Does this make sense? What do you think?

[ItsNickBarry]

So my goal is to allow separate "abi exporters"

This part is really interesting. As long as the abiExporter config key can accept either an object or an array of objects, I think it's good to add. Started an implementation in PR #27. Granted you access the the repository so that you can push to the development branch

I'm still not completely sure about this implementation of rename, but I think we can merge soon if I don't think of anything better.

[ItsNickBarry]

It seems like rename is kind of redundant if each config object can specify a path. Is it terribly inconvenient to just nest the exports in different directories?

[ItsNickBarry]

The filter function is similar to something brought up in #20. I was against it at the time, but if we can support multiple use cases with a single generic callback function, I think it's okay.

[phated]

It seems like rename is kind of redundant if each config object can specify a path. Is it terribly inconvenient to just nest the exports in different directories?

I think that's fair. Putting things in different directories makes things a little weird for our published packages. I wonder if path could be changed to support what I was trying to do with rename? Maybe it could take a function, but if it is a string, it uses the current logic.

[phated]

@ItsNickBarry I dug into the path suggestion above and found that it actually makes the clear-abi task really weird. Are you sure we can't keep the rename around while also keeping flat? Or are you completely against having this at all?

[ItsNickBarry]

Let's keep both rename and flat, and throw an error if both are set. I will make a final decision about flat before 3.0.0 release when I republish as @solidstate/hardhat-abi-exporter.

[phated]

@ItsNickBarry I finally had a chance to update this as we discussed. Can you take a look?

I'd like to get this feature released so I can update the Dark Forest codebase.

[ItsNickBarry]

Will review as soon as possible.

[ItsNickBarry]

@phated Fixed the case where neither flat nor rename is specified, and in the process allowed both values to be set as long as flat is false. If everything looks good to you, will merge and publish.

[phated]

Just one question about the validation, but otherwise looks great

[phated]

Do we want to re-add this boolean validation then?

[ItsNickBarry]

Yes

[ItsNickBarry]

Done

[ItsNickBarry]

Released as 2.9.0.