Skip to content

provider: rpc and signer#234

Merged
karlfloersch merged 35 commits intoethereum-optimism:masterfrom
tynes:provider
Sep 13, 2020
Merged

provider: rpc and signer#234
karlfloersch merged 35 commits intoethereum-optimism:masterfrom
tynes:provider

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented Aug 26, 2020

Description

Adds a new package that includes an OptimismProvider. This provider includes a signer that uses the eth_sign signature hashing scheme.

Questions

  • What are the main api that app developers use today with ethers providers, and is there test coverage for them?

Metadata

Fixes

  • Fixes # [Link to Issue]

Contributing Agreement

@tynes tynes changed the title WIP: provider: rpc and signer provider: rpc and signer Sep 8, 2020
@tynes tynes marked this pull request as ready for review September 8, 2020 20:12
Copy link
Contributor

@karlfloersch karlfloersch left a comment

Choose a reason for hiding this comment

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

This looks awesome! Very clean & the 700 line ERC20 and 650 line yarn.lock makes this PR actually deceptively small (especially considering all that it's doing). This is awesome! LGTM~

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh something that could be cool to test is sending a transaction with something like this:

erc20.transfer(...)

The difficulty here is just that at the moment that transaction would only succeed in L2Geth but right now we don't have integration tests of course.

Do you have any tests like this? Would this be something we'd want in these unit tests or should we wait until we have the integration tests repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good thinking! I'm leaning towards getting that into the integration tests repo so that I can focus on some other tickets in the meantime

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great

@karlfloersch
Copy link
Contributor

Oh it looks like there's a yarn.lock conflict lol so gotta fix that before merge of course

@tynes
Copy link
Contributor Author

tynes commented Sep 10, 2020

I fixed the conflict with yarn.lock and also added the package to lerna.json and turned off the integration tests that still exist in this repo.

@tynes
Copy link
Contributor Author

tynes commented Sep 10, 2020

Also removed test/data/ERC20.json for now, this can be revisited in the integration testing repo. Anything else that needs to be done to get this package into a place where its good to be published? Is optimism-provider an ok name for its npm package? Its a bit redundant if its @eth-optimism/optimism-provider, so what do you think about renaming it to just provider so its @eth-optimism/provider?

@tynes
Copy link
Contributor Author

tynes commented Sep 10, 2020

Renamed the package to provider so that it can be @eth-optimism/provider and also added a little usage blurb to the README

@karlfloersch
Copy link
Contributor

@tynes I think this is good to go and totally approve of renaming it to just provider

@tynes
Copy link
Contributor Author

tynes commented Sep 11, 2020

Fixed the linting errors

@karlfloersch
Copy link
Contributor

Awesome!!!

@karlfloersch karlfloersch merged commit bbf5572 into ethereum-optimism:master Sep 13, 2020
snario pushed a commit that referenced this pull request Apr 14, 2021
OptimismBot pushed a commit that referenced this pull request Jan 30, 2025
xibao-nr pushed a commit to node-real/combo-optimism that referenced this pull request Feb 19, 2025
Zena-park added a commit to tokamak-network/optimism that referenced this pull request Dec 30, 2025
theochap pushed a commit that referenced this pull request Jan 15, 2026
### Description

Breaks out the small batch decoding fix from #234.
theochap pushed a commit that referenced this pull request Jan 21, 2026
### Description

Breaks out the small batch decoding fix from #234.
emhane added a commit that referenced this pull request Feb 3, 2026
Seems like when `reth-optimism-trie` depends on `reth-db-api`
indirectly, the doctest crate couldn’t resolve `reth_db_api::…` and
failing.

Closes #233

Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com>
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