Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Respect solc version when importing ABI JSON files inside Solidity code #4365

Merged
merged 2 commits into from
Nov 21, 2021

Conversation

gnidan
Copy link
Contributor

@gnidan gnidan commented Oct 13, 2021

This PR upgrades abi-to-sol to ^0.5.1, taking advantage of the new functionality in that package to generate version-specific Solidity output.

In order to accommodate this new functionality, we need to know what Solidity version to generate, and so this PR builds on the work of #4420 by hooking the compiler information into the ABI ResolverSource.

Mostly straightforward, but I noticed one gotcha: when resolver.resolve() receives compiler info, it likely receives a version string containing the full commit and target info. This PR ensures the ABI ResolverSource strips that off because otherwise abi-to-sol throws an exception.

cds-amal
cds-amal previously approved these changes Oct 14, 2021
Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

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

Looks good @gnidan . We'll know if it breaks soon enough :)

@cds-amal
Copy link
Member

(This shouldn't break anything that isn't already broken, but we might want to do some work to pass the solc version into the resolver, so that abi-to-sol can generate output specifically for that version)

This is a good idea! Can you make an issue so we don't lose it?

@gnidan
Copy link
Contributor Author

gnidan commented Oct 14, 2021

Yep seems more is needed here. Oops!

@gnidan gnidan marked this pull request as draft October 14, 2021 03:18
@gnidan gnidan changed the base branch from develop to resolver/compiler-info November 10, 2021 22:52
@gnidan gnidan force-pushed the resolver/compiler-info branch 2 times, most recently from c9968ec to d57ab0c Compare November 13, 2021 01:58
@gnidan gnidan added ⏸️ merge after target this PR targets another PR; that other PR should be merged first dependencies Pull requests that update a dependency file labels Nov 16, 2021
@haltman-at
Copy link
Contributor

haltman-at commented Nov 18, 2021

This no longer needs to be merge-after-target, right? :) Oops, silly me, the thing that it depends on isn't merged yet, got ahead of myself.

Base automatically changed from resolver/compiler-info to develop November 19, 2021 22:49
@haltman-at
Copy link
Contributor

OK, now this no longer needs to be merge-after-target, right? :)

@gnidan gnidan removed the ⏸️ merge after target this PR targets another PR; that other PR should be merged first label Nov 20, 2021
- Ensure we pass solc version info to abi-to-sol
- Strip off `+commit.<hash>.<build>` suffix from argument
@gnidan gnidan changed the title Upgrade abi-to-sol to ^0.5.0 Upgrade abi-to-sol to ^0.5.1 Nov 20, 2021
@gnidan gnidan requested a review from cds-amal November 20, 2021 03:32
@gnidan gnidan marked this pull request as ready for review November 20, 2021 03:32
@gnidan gnidan dismissed cds-amal’s stale review November 20, 2021 03:34

This approval was from a time when we thought this upgrade would be easy

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

Seems good, just two quick comments.

packages/resolver/lib/sources/abi.ts Show resolved Hide resolved
packages/resolver/lib/sources/abi.ts Show resolved Hide resolved
Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

Cool, seems good to me!

@gnidan
Copy link
Contributor Author

gnidan commented Nov 20, 2021

Thanks! Will merge when green.

@gnidan gnidan changed the title Upgrade abi-to-sol to ^0.5.1 Respect Solidity version differences when importing ABI JSON files Nov 20, 2021
@gnidan gnidan added enhancement and removed dependencies Pull requests that update a dependency file labels Nov 20, 2021
@gnidan
Copy link
Contributor Author

gnidan commented Nov 20, 2021

(Also reworded this to be an enhancement, rather than a dependency update, to be more explicit about the reasoning behind this upgrade.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants