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

Fix estimate gas #161

Merged
merged 14 commits into from
Aug 3, 2023
Merged

Fix estimate gas #161

merged 14 commits into from
Aug 3, 2023

Conversation

cryptoAtwill
Copy link
Contributor

@cryptoAtwill cryptoAtwill commented Jul 25, 2023

When sending transactions, we cannot estimate gas due to invalid serialisation error. This is because ethers-rust requires the transaction type to be zero padded hex string while hardhat does not enforce zero padding.

We propose a temporary solution as follows:
Since we only use Eip1559TransactionRequest, we created a new TypedTransaction that uses only Eip1559TransactionRequest. When deserializing, we use alias that handles both zero padding and non zero padding.

See gakonst/ethers-rs#2518 for ethers fix for this issue.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

Cargo.toml Outdated
@@ -63,12 +63,12 @@ tracing-subscriber = "0.3"


# Stable FVM dependencies from crates.io
fvm = { version = "3.4", default-features = false } # no opencl feature or it fails on CI
fvm = { version = "=3.3", default-features = false } # no opencl feature or it fails on CI
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have use ~3.3 but I doubt they will release patches retrospectively so it's okay.

docs/running.md Outdated
We can try query the chain id by:
```shell
curl -X POST -i -H 'Content-Type: application/json' -d '{"jsonrpc":"2.0","id":0,"method":"eth_chainId","params":[]}' http://localhost:8545
```
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to mention it at the bottom of the doc that together with this container we can run the ethers example as well to see a fully Ethereum compliant interaction with the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cryptoAtwill, do you mind picking up this one?

@adlrocha
Copy link
Contributor

Sorry, folks. I didn't know this PR was already a thing and I was working on this same branch to build upon @cryptoAtwill changes to TypedTransaction, I didn't know they were final. I added some changes to support LegacyTransactions (nedded for Metamask and Remix support)

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

call will need this fix as well.

@adlrocha
Copy link
Contributor

call will need this fix as well.

I had it in the backlog, but if you don't mind, I will solve this one on top of #163 so I can test the end-to-end with Remix.

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Woot!

adlrocha and others added 5 commits July 31, 2023 20:10
Signed-off-by: Alfonso de la Rocha <adlrocha@tutamail.com>
* add some traces

* improve gas estimation logic

* clean comments

* fix linter

* remove unused println

* FM-163: address review and prevent panic errors

* FIX: make linter happy

* FM-163: clean code and finish review

* FM-163: rm prints

* FM: add transaction compat to call query (#170)

* FM-163: modify cache semantics in call

* FM-163: fix gas_fee_cap and add fvm settings to config

* FM-163: fix use_cache and config

---------

Signed-off-by: Alfonso de la Rocha <adlrocha@tutamail.com>
Signed-off-by: Alfonso de la Rocha <adlrocha@tutamail.com>
@adlrocha adlrocha merged commit f8f32b8 into master Aug 3, 2023
5 checks passed
@adlrocha adlrocha deleted the fix_estimate_gas branch August 3, 2023 17:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants