Skip to content

[Breaking API Change] Use list instead of set for filenames and contracts_names; change filename ordering#432

Closed
samalws-tob wants to merge 1 commit intocrytic:devfrom
samalws-tob:ordering3
Closed

[Breaking API Change] Use list instead of set for filenames and contracts_names; change filename ordering#432
samalws-tob wants to merge 1 commit intocrytic:devfrom
samalws-tob:ordering3

Conversation

@samalws-tob
Copy link
Copy Markdown
Contributor

(See bolded for TODOs that I could use some advice on)

This PR:

  • Uses lists instead of sets for filenames and contracts_names.
  • Changes code for each platform so that filenames is ordered in the same way as targets_json["sources"]. In waffle and embark, this results in new code. Otherwise it's just moving code around.
  • Deletes redundant code in embark and platform/standard.py that added filenames to the list.

It fixes:

  • source_list being ordered differently than the source list outputted by the compiler, resulting in source maps being incorrect, resulting in echidna producing bad coverage reports.
  • Contracts list being ordered differently each time, resulting in nondeterministic outputs

Testing:

  • Tested with hardhat on multiple projects, including projects where the current version of crytic_compile works correctly with echidna and projects where it works incorrectly with echidna (incorrect ordering of filenames, producing bad coverage).
  • Tested on a hardhat project with multiple versions of hardhat (2.1, 2.8, 2.14).
  • Tested on a forge project where the current version of crytic_compile works incorrectly with echidna.
  • Tested on an example waffle project, since I added a few lines to waffle.py instead of just moving things around
  • Intended to test on embark since I added and removed a few lines there, but couldn't install embark in order to test it. Tried on Mac, NixOS, and WSL running Ubuntu. Help/advice here would be appreciated. I don't think testing here outside of CI tests is strictly necessary since I can't see any way my new code breaks anything that wouldn't be caught by CI; the only possibility seems to be that targets_loaded["sources"] doesn't exist and my new code never gets called, resulting in filenames potentially having the wrong order.
  • Haven't tested out other frameworks yet. Should I go through them one by one, installing and making projects and crytic-compiling? Should I just trust the CI to catch any bugs? In general, code is mostly moved around rather than being rewritten, so I'd imagine that there aren't any deep bugs that CI couldn't catch.

@samalws-tob
Copy link
Copy Markdown
Contributor Author

#436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant