-
Notifications
You must be signed in to change notification settings - Fork 746
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
[pallet-revive] eth-prc fix geth diff #6608
Conversation
c434487
to
1f069da
Compare
1f069da
to
716f40a
Compare
/cmd prdoc --audience runtime_dev --bump minor |
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.
Just some nits
substrate/frame/revive/rpc/examples/js/contracts/ErrorTester.sol
Outdated
Show resolved
Hide resolved
substrate/frame/revive/rpc/examples/js/contracts/ErrorTester.sol
Outdated
Show resolved
Hide resolved
} | ||
match function_selector { | ||
// assert(false) | ||
[0x4E, 0x48, 0x7B, 0x71] => { |
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.
Does the alloy
crate or something similar provide those types?
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.
Not sure, this is inspired by the go-ethereum source code
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.
We don't depend on alloy, so not really worth adding a new dependency just for that for now
All GitHub workflows were cancelled due to failure one of the required jobs. |
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.
I think you need to disable the tests written in Solidity for CI to pass. The 64bit PR broke them. Or you rebuilt them with a new revive.
Didn't you do it in your PR already? |
I did. I thought you added new tests here that need to be disabled. But the CI error is from something else. |
Ah i was going to add a few tests will do that in a follow up |
@pgherveou I think you might have overlooked in
to
|
Thanks @cmichi, this new type barely acts as a more meaningful replacement for |
Add tests for #6608 fix paritytech/contract-issues#12 --------- Co-authored-by: command-bot <>
* Add a bunch of differential tests to ensure that responses from eth-rpc matches the one from `geth` - These [tests](https://github.com/paritytech/polkadot-sdk/blob/pg/fix-geth-diff/substrate/frame/revive/rpc/examples/js/src/geth-diff.test.ts) are not run in CI for now but can be run locally with ```bash cd revive/rpc/examples/js bun test ``` * EVM RPC server will not fail gas_estimation if no gas is specified, I updated pallet-revive to add an extra `skip_transfer` boolean check to replicate this behavior in our pallet * `eth_transact` and `bare_eth_transact` api have been updated to use `GenericTransaction` directly as this is what is used by `eth_estimateGas` and `eth_call` ## TODO - [ ] Add tests the new `skip_transfer` flag --------- Co-authored-by: GitHub Action <[email protected]> Co-authored-by: Alexander Theißen <[email protected]>
geth
EVM RPC server will not fail gas_estimation if no gas is specified, I updated pallet-revive to add an extra
skip_transfer
boolean check to replicate this behavior in our palleteth_transact
andbare_eth_transact
api have been updated to useGenericTransaction
directly as this is what is used byeth_estimateGas
andeth_call