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.

Sign up for GitHub

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

Jump to bottom

Ethpm v3 #1652

Merged
merged 15 commits into from
Jul 7, 2020
Merged

Ethpm v3 #1652

merged 15 commits into from
Jul 7, 2020

Conversation

njgheorghita
Copy link
Contributor

@njgheorghita njgheorghita commented May 15, 2020 •
edited
Loading

What was wrong?

The ethpm module and web3.pm needed updating to support the latest ethpm version. The most significant changes to an ethpm schema include....

  • snake_case -> camelCase
  • package_name -> name
  • manifest_version -> manifest
  • compilers was added as a new top-level field
  • name and version are no longer strictly required fields

How was it fixed?

We decided to make a hard update from v2 -> v3 (as opposed to deprecating and gradually phasing in v3 support) for reasons mentioned in a comment below.

  • Updated v2 schema to v3 and all examples
  • Updated code & tests to support new manifests/schema
  • Submoduled ethpm-spec which allowed us to remove a ton of redundant assets

Todo:

  • Add flag / check for v2 packages to announce deprecated support
  • Update web3 docs
  • Update docs.ethpm.com
  • Update ethpm/tools
  • Update web3.pm
  • Submodule ethpm-spec
  • Remove outdated assets
  • Add entry to the release notes

Cute Animal Picture

image

@njgheorghita njgheorghita force-pushed the ethpm-v3 branch 3 times, most recently from ba04247 to 81e61d8 Compare May 18, 2020 15:55
@njgheorghita njgheorghita force-pushed the ethpm-v3 branch 15 times, most recently from 732b1d3 to 52b8fe8 Compare June 30, 2020 19:23
@njgheorghita njgheorghita force-pushed the ethpm-v3 branch 7 times, most recently from bb82277 to d7ad81c Compare July 1, 2020 21:38
pipermerriam
pipermerriam reviewed Jul 1, 2020
View reviewed changes
Copy link
Member

@pipermerriam pipermerriam left a comment

Choose a reason for hiding this comment

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

Just to check, this is going to intentionally deprecate support for v2 and only support v3?

@njgheorghita
Copy link
Contributor Author

@pipermerriam Yup, we landed on this a while back b/c it's behind the enable_unstable_pm_api flag, there's low V2 adoption, and it helps simplify the update vs. supporting both v2 & v3 simultaneously.

@njgheorghita njgheorghita force-pushed the ethpm-v3 branch 4 times, most recently from b3ea18e to 99a97f2 Compare July 1, 2020 22:36
@njgheorghita njgheorghita marked this pull request as ready for review July 1, 2020 22:54
@njgheorghita njgheorghita changed the title [WIP] Ethpm v3 Ethpm v3 Jul 1, 2020
@njgheorghita
Copy link
Contributor Author

@pipermerriam @marcgarreau Okey dokey, this turned into a doozy of a pr. But most of the changes are fairly cosmetic, and the majority of files changed are simply removing redundant assets. I'm happy to hop on a call and walk-through the changes. If anybody is uncomfortable with merging this branch directly into master, I'm definitely open to suggestions / alternative approaches.

wolovim
wolovim approved these changes Jul 3, 2020
View reviewed changes
Copy link
Member

@wolovim wolovim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There's a lot in there, but I didn't see anything concerning on a "pragmatic" read through.

Edit: don't forget the newsfragment ;)

web3/tools/pytest_ethereum/linker.py Outdated
@@ -100,10 +100,11 @@ def link(contract: ContractName, linked_type: str, package: Package) -> Package:
"so it is not a valid contract type for link()"
)
linked_factory = unlinked_factory.link_bytecode({linked_type: deployment_address})
# ???
Copy link
Member

Choose a reason for hiding this comment

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

???

tests/ethpm/validation/test_manifest.py Outdated


@pytest.mark.parametrize("version", (1, 2, "1" "3", b"3"))
@pytest.mark.parametrize("version", (2, 3, "2" "3", b"3"))
Copy link
Member

Choose a reason for hiding this comment

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

Is ethpm/3 the only acceptable answer here?

Copy link
Member

Choose a reason for hiding this comment

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

And should "2" and "3" be separated by a comma?

Copy link
Contributor Author

@njgheorghita njgheorghita Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is ethpm/3 the only acceptable answer here?

Yup, that's the only valid version string for an ethpm v3 manifest.

pipermerriam
pipermerriam reviewed Jul 3, 2020
View reviewed changes
ethpm/backends/ipfs.py Outdated
@@ -154,7 +154,7 @@ def base_uri(self) -> str:
MANIFEST_URIS = {
"ipfs://QmVu9zuza5mkJwwcFdh2SXBugm1oSgZVuEKkph9XLsbUwg": "standard-token",
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why these didn't change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh good catch, well it's because this is just a small cache for the DummyIPFSBackend and DummyIPFSBackend.fetch_uri_contents() simply fetches the local asset rather than fetching it from IPFS, so the hash isn't really used in any meaningful way. But, they're all updated now.

@njgheorghita njgheorghita mentioned this pull request Jul 6, 2020
Merged
@njgheorghita njgheorghita merged commit b680d52 into ethereum:master Jul 7, 2020
@njgheorghita njgheorghita deleted the ethpm-v3 branch July 7, 2020 16:04
@njgheorghita njgheorghita mentioned this pull request Jul 7, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pipermerriam pipermerriam pipermerriam left review comments

@wolovim wolovim wolovim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@njgheorghita @pipermerriam @wolovim