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

cmd/abigen: Sanitize vyper's combined json names #20419

Merged
merged 2 commits into from
Dec 16, 2019

Conversation

gballet
Copy link
Member

@gballet gballet commented Dec 3, 2019

This is a fix for #20418

Solidity has a combined_json output with the following schema:

{
 "filename:contractname": { ... }
 ...
}

In the case of vyper, it's only:

{
 "filename": { ... }
 ...
}

This PR sanitizes the output if such a case is detected. I will also open an issue in the vyper repo.

@gballet
Copy link
Member Author

gballet commented Dec 3, 2019

Vyper issue created at vyperlang/vyper#1754

@gballet gballet requested a review from rjl493456442 December 3, 2019 07:35
@rjl493456442
Copy link
Member

The only concern I have is: for library reference calculation.

Since in the solidity, we use the whole path like /sources/main.sol:A to calculate the libPattern. Does it still work for vyper?

@gballet gballet force-pushed the fix-vyper-combined-json branch from fe0899d to 02c8bf5 Compare December 16, 2019 09:29
Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

LGTM

@gballet gballet merged commit 275cd49 into ethereum:master Dec 16, 2019
@gballet gballet added this to the 1.9.10 milestone Dec 16, 2019
enriquefynn pushed a commit to enriquefynn/go-ethereum that referenced this pull request Mar 10, 2021
* cmd/abigen: Sanitize vyper's combined json names

* Review feedback: handle full paths
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.

2 participants