Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(solc): Use IndexedPathBuf for CompilerInput Sources #1151

Closed
wants to merge 4 commits into from

Conversation

pyk
Copy link
Contributor

@pyk pyk commented Apr 16, 2022

Motivation

Based on foundry-rs/foundry#1283 it seems that Etherscan is not automatically detect main contract to be displayed as the first source on the list.

After learning how hardhat-etherscan verification works turns out that hardhat always place the main contract as the first key of the sources.

This behaviour is currently not possible in ethers-solc due to CompilerInput sources is using BTreeMap<PathBuf, Source>. When compiler input is serialized as JSON, it always ordered alphabetically (PathBuf default), not by their insertion order.

Solution

This changes propose Sources to be defined as BTreeMap<IndexedPathBuf, Source> instead of BTreeMap<PathBuf, Source>. IndexedPathBuf contains index field which we can use to determine how the key is sorted by just simply assign the index value.

In the case of verification, the sources key will be ordered by source code for main contract first then followed by the imported source codes.

Results:

(SimpleERC20 is on top of the list and followed by the imports)

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems that this is primarily for verification?

Are there any other advantages, because this touches a lot of internal code and indexedpath is only needed for verification?

I've been thinking a bit about how to implement this, but haven't gotten around to writing it down yet.

What if we make the standard_json function return a new type that acts like a CompilerInput but seriealises with sorted sources? basically your IndexedPath apporach

the index would probably come from the graph, imported_nodes I think? So I think we can make sources: Vec<(PathBuf, Source), which is serialised as map

ref https://serde.rs/impl-serialize.html#serializing-a-sequence-or-map

@pyk
Copy link
Contributor Author

pyk commented Apr 16, 2022

it seems that this is primarily for verification?

Yess

Are there any other advantages, because this touches a lot of internal code and indexedpath is only needed for verification?

I believe this only useful for verification only

What if we make the standard_json function return a new type that acts like a CompilerInput but seriealises with sorted sources? basically your IndexedPath apporach

Do you mean standard_json return trait or just new struct that behave like CompilerInput?

I think if we can change the return of standard_json to serde_json::Value it will much simpler using json! macro (which can be deserialized to CompilerInput)

@gakonst
Copy link
Owner

gakonst commented Apr 18, 2022

Yeah I'd say let's try to make it sort only when serializing in a non invasive way if possible!

@pyk
Copy link
Contributor Author

pyk commented Apr 18, 2022

@gakonst ok, noted 🙇

@pyk
Copy link
Contributor Author

pyk commented Apr 23, 2022

replaced by #1169 , thanks @mattsse sorry I haven't finished this yet. i will pickup other foundry issues this weekend 🙇

@pyk pyk closed this Apr 23, 2022
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.

3 participants