-
Notifications
You must be signed in to change notification settings - Fork 790
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
Update EthersStateManager
to be RpcStateManager under the hood
#3167
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
EtherStateManager
blockchain supplemental workEthersStateManager
to be RpcStateManager under the hood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments
if (typeof opts.provider === 'string') { | ||
this._provider = new ethers.JsonRpcProvider(opts.provider) | ||
} else if (opts.provider instanceof ethers.JsonRpcProvider) { | ||
if (typeof opts.provider === 'string' && opts.provider.startsWith('http')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but just noting: we thus do not have websocket support. (Also does not seem that fetchFromProvider
accepts this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. This only works over HTTP. If anyone ever asks about websocket support, we can explore it. I'm not sure if node's native fetch
instance supports WS anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ws are added in future you might need to use an isomorphic library or dependency inject it to get it to work in both client and node
I don't understand, tests and linter fail locally (also after a fresh build), but not on CI? 😓 |
Nvm, did not pull 🤦 |
b8daaa6
to
58eb39c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing! Big improvement from pulling in viem or ethers.js to do something that can be done with this much less code.
I noticed the error handling on fetchFromProvider was less than ideal so I opened a PR to improve it: #3171
if (typeof opts.provider === 'string') { | ||
this._provider = new ethers.JsonRpcProvider(opts.provider) | ||
} else if (opts.provider instanceof ethers.JsonRpcProvider) { | ||
if (typeof opts.provider === 'string' && opts.provider.startsWith('http')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ws are added in future you might need to use an isomorphic library or dependency inject it to get it to work in both client and node
packages/statemanager/test/testdata/providerData/mockProvider.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits
e02ed1b
to
d0b4bd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great work 🙏 😄
Several improvements for
EthersStateManager
ethers
dependency and replace with raw JSON RPC calls similar toBlock.fromJsonRpcProvider
blockchain
class that mimics existingevm
DefaultBlockchain
behavior - has benefit of fixing issue whereBLOCKHASH
opcode wouldn't work right underEthersStateManager
EthersStateManager
tests with better mocks for JSON-RPC provider