fix(ethers-solc/ConfigurableArtifacts): output methodIdentifiers by default

[meetmangukiya]

Related foundry-rs/foundry#1497

Motivation

As per https://github.com/gakonst/foundry/blob/0062360706e77a155856e128c3e250264c75f48c/config/src/lib.rs#L199-L214 methodIdentifiers should be enabled as extra output by default as well in addition to abi, bytecode and deployedBytecode.

Solution

Making it default in ethers as other fields, but can also inject methodIdentifiers as extra_output in Config::extra_output.

PR Checklist

• Added Tests
• Added Documentation
• Updated the changelog

[mattsse]

lgtm

[gakonst]

I'm not sure if this is the right solve here, as it doesn't let us turn off the method identifiers. Also if we apply the same solve to make forge inspect work with every variant, don't we end up turning everything on by default? Cc @mattsse

[meetmangukiya]

For all other variants I believe, it will not use the cached build when you run forge inspect after forge build. This was buggy because we consider methodIdentifiers as default like abi, bytecode and deployedBytecode so cache is considered valid.

[meetmangukiya]

The methodIdentifiers case is buggy because to solc settings we used to provide all the current 4 defaults (including the method identifiers) https://github.com/foundry-rs/foundry/blob/master/config/src/lib.rs#L639-L652 but the artifact handler was passed only the args https://github.com/foundry-rs/foundry/blob/master/config/src/lib.rs#L611-L613 which in case of plain forge build is empty and method identifiers wouldn't be included. While the cache resolver still thinks the current build was with methodIdentifiers, but our artifact handler never wrote that out.

[mattsse]

in the artifact,

method_identifiers: Option<BTreeMap<String, String>>

but in the evm object

#[serde(default, skip_serializing_if = "::std::collections::BTreeMap::is_empty")]
pub method_identifiers: BTreeMap<String, String>,

we should both unify to BTreeMap I think.

having had a closer look at this https://github.com/foundry-rs/foundry/blob/master/config/src/lib.rs#L611-L613

the right way to fix this is probably to adjust the ConfigurableArtifacts and ensure the method Ids is included so this can actually be true

https://github.com/gakonst/ethers-rs/pull/1266/files#diff-e6653c43399a28fd5658fc73570833cc1e18950040a47795f6551a02a9b280f5L297

?

[meetmangukiya]

Yep, thought of adding it there, but the other 3 defaults were made default in ethers, so went that way for consistency

[gakonst]

ACK - let's merge this then to unblock the Foundry issue, and think about the caching issue and replacing Option<BTreeMap> with BTreeMap in a follow up