Name clash with local folder and dependency causes contract verification issue

[derekpierre]

Environment information

$ ape --version
0.8.14

$ ape plugins list
Installed Plugins
  etherscan    0.8.3
  infura       0.8.1
  polygon      0.8.0
  solidity     0.8.3

• Python Version: 3.11.9
• OS: macOS

What went wrong?

I was trying to do verification of our Coordinator contract (https://github.com/nucypher/nucypher-contracts/blob/main/contracts/contracts/coordination/Coordinator.sol) on polygon amoy, and ran into the following problem.

In our code base, we have a local threshold sub-folder which includes a local dependency used by the Coordinator contract (https://github.com/nucypher/nucypher-contracts/blob/main/contracts/contracts/coordination/Coordinator.sol)

import "../../threshold/ITACoChildApplication.sol";

The tree structure can be observed here, https://github.com/nucypher/nucypher-contracts/tree/main/contracts.

However, we also have a dependency on a threshold module in our ape-config.yaml:

  - name: threshold
    github: threshold-network/solidity-contracts
    version: 1.2.1

This name clash seems to cause issues when creating import remappings i.e. instead of using the local threshold local folder path for the import statement, the contract is searched for in the threshold dependency instead and is not found, and raises an exception.

The cause of the issue seems to be related to the following. in _create_import_mapping, https://github.com/ApeWorX/ape-solidity/blob/main/ape_solidity/_models.py#L57, two entries are added to the dictionary:

"@threshold" -> <threshold dependency path>
"threshold" -> <same threshold dependency path as above>

Therefore, when checking whether the import is a for a local file or dependency here, https://github.com/ApeWorX/ape-solidity/blob/main/ape_solidity/_models.py#L202,

for check_remap_key, check_remap_value in import_remapping.items():
            if check_remap_key not in self.value:
                continue

            valid_matches.append((check_remap_key, check_remap_value))

The string "threshold" is indeed found in the import line import "../../threshold/ITACoChildApplication.sol" and therefore, it is supposedly a "valid match" and assumed that the file is in the dependency path instead of the local path. This causes the exception that the imported file, ITACoApplication.sol, cannot be found

It's strange because if I wanted to use the dependency path in the import, I would use import @threshold/.... What's the reason for 2nd entry in the import remapping dictionary that does not contain the @ symbol? Legacy reasons, perhaps...? Paths within the dependency module itself - although it presumably wouldn't need to use its own folder path including threshold...? (Just spitballing here 😅 )

How can it be fixed?

The local fix that worked for me was:

diff --git a/ape_solidity/_models.py b/ape_solidity/_models.py
index 74a780c..00137c6 100644
--- a/ape_solidity/_models.py
+++ b/ape_solidity/_models.py
@@ -54,7 +54,7 @@ def _create_import_remapping(project: ProjectManager) -> dict[str, str]:

         for unpacked_dep in dep.unpack(project.contracts_folder / ".cache"):
             main_key = key_map.get(unpacked_dep.name)
-            keys = (main_key,) if main_key else (f"@{unpacked_dep.name}", unpacked_dep.name)
+            keys = (main_key,) if main_key else (f"@{unpacked_dep.name}",)
             for _key in keys:
                 if _key not in remapping:
                     remapping[_key] = get_cache_id(unpacked_dep)

i.e. only the entry with the @ symbol is stored. That being said, it worked for me, and I'm unsure how general this solution is.

I figured you guys would have more context about that change as to whether it is legit, or perhaps have an alternative suggestion.

[linear[bot]]

APE-1808 Name clash with local folder and dependency causes contract verification issue

[antazoey]

Since different projects tend to use both, the intent was that it tries both and discards the one not being used

[derekpierre]

Since different projects tend to use both, the intent was that it tries both and discards the one not being used

Would a possible alternative be hardening the following check, https://github.com/ApeWorX/ape-solidity/blob/main/ape_solidity/_models.py#L212, trying raw_value and biasing local path and checking for existence there before checking external dependency and the import remapping? It seems that self.value property changes given the original case that a valid_match was found in _resolve_import_remapping.

[antazoey]

@derekpierre Potentially that's a solution. Either way, we should be able to get this resolved. Local paths are local paths, after all.

[antazoey]

^ linked PR so far just creates the failing condition. Working on actual fix now.

Edit: OK PR is ready for testing now.