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

ci: fix nonce manager test #2014

Merged
merged 3 commits into from
Jan 6, 2023

Conversation

0xMelkor
Copy link
Contributor

@0xMelkor 0xMelkor commented Jan 5, 2023

Motivation

CI breaks on NonceManager test on the Goerli network.

Solution

Switched the network from Goerli to Anvil to work in a sand-box.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog
  • Breaking changes

@0xMelkor
Copy link
Contributor Author

0xMelkor commented Jan 5, 2023

Also ethers-middleware/tests/signer.rs fails on Goerli.
I'll check it

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

at least one failing test less,
testing against anvil will be more stable than using a live network.

thanks!

@0xMelkor
Copy link
Contributor Author

0xMelkor commented Jan 5, 2023

at least one failing test less, testing against anvil will be more stable than using a live network.

Hi @mattsse , does Anvil support EIP1559? I get this error on send:

{ err: Error("invalid length 1, expected a (both 0x-prefixed or not) hex string or byte array containing 8 bytes", line: 1, column: 858), text: "{\"baseFeePerGas\":\"0x3b9aca00\",\"difficulty\":\"0x0\",\"extraData\":\"0x\",\"gasLimit\":\"0x1c9c380\",\"gasUsed\":\"0x5208\",\"hash\":\"0x946b047adbc3accea9304628094709e7f246f38d998b09469d01a0bdf3a9fbd9\",\"logsBloom\":\"0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000\",\"miner\":\"0x0000000000000000000000000000000000000000\",\"mixHash\":\"0x0000000000000000000000000000000000000000000000000000000000000000\",\"nonce\":\"0x0\",\"number\":\"0x2\",\"parentHash\":\"0x9fd5237a990c0cccaed76e7f3957d23f0e6653c8fad17f3badaf0f31c4267785\",\"receiptsRoot\":\"0x056b23fbba480696b65fe5a59b8f2148a1299103c4f57df839233af2cf4ca2d2\",\"sealFields\":[\"0x0000000000000000000000000000000000000000000000000000000000000000\",\"0x0000000000000000\"],\"sha3Uncles\":\"0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347\",\"size\":\"0x260\",\"stateRoot\":\"0x3d2fc4451b81b998ab24989c32db79421b3ae7e7d39039ac902de26f05178edd\",\"timestamp\":\"0x63b6c185\",\"totalDifficulty\":null,\"transactions\":[\"0x6e631fa20f50ef664f83265c95489b460fcffcde08f3097f723b2f4d4a8359e8\"],\"transactionsRoot\":\"0x5aa9c0fa38efd734933a8d89577e06f1dfc3074474220c27988dd3ec0d270834\",\"uncles\":[]}" }

@mattsse
Copy link
Collaborator

mattsse commented Jan 5, 2023

this is a serde error,

but would need more context in order to debug this, likely U64 related or something.

please open a separate issue on foundry ideally with a repro, or if you're up to it, you can debug yourself. It'd start with an e2e test ref
https://github.com/foundry-rs/foundry/blob/master/anvil/tests/it/transaction.rs

@0xMelkor
Copy link
Contributor Author

0xMelkor commented Jan 5, 2023

please open a separate issue on foundry ideally with a repro, or if you're up to it, you can debug yourself. It'd start with an e2e test ref https://github.com/foundry-rs/foundry/blob/master/anvil/tests/it/transaction.rs

Hi @mattsse,
this is an ethers-rs regression introduced at PR #1980. I reverted changes locally and e2e tests work.
I'd close this PR, and maybe ask @prestwich to fix here

Let me know guys if I can help in some way.

@mattsse
Copy link
Collaborator

mattsse commented Jan 5, 2023

could you please describe what exactly is the issue, and why it is happening?

@0xMelkor
Copy link
Contributor Author

0xMelkor commented Jan 5, 2023

could you please describe what exactly is the issue, and why it is happening?

Investigating..

@0xMelkor
Copy link
Contributor Author

0xMelkor commented Jan 5, 2023

The current code is supposed to update the value of inner.max_priority_fee_per_gas.

  inner
    .max_priority_fee_per_gas
    .map(|tip| std::cmp::min(tip, *mfpg))
    .get_or_insert(max_priority_fee_per_gas);

It looks good, but for some reason get_or_insert doesn't update the value and a None value is left.
Please note that get_or_insert is marked as unstable.

Changing the code to this updates the value, and tests run correctly.

inner.max_priority_fee_per_gas = inner
    .max_priority_fee_per_gas
    .map(|tip| std::cmp::min(tip, *mfpg))
    .or(Some(max_priority_fee_per_gas));

@prestwich
Copy link
Collaborator

Please note that get_or_insert is marked as unstable.

const version is unstable. get_or_insert has been stable since 1.20

It looks good, but for some reason get_or_insert doesn't update the value and a None value is left.

Reviewing the code:

inner
    .max_priority_fee_per_gas
    .map(|tip| std::cmp::min(tip, *mfpg))
    .get_or_insert(max_priority_fee_per_gas);

My mistake is that the code is written as if .map() is transformative. I.e. as if it were map(&mut self: Option<T>, f: Fn(T) -> U) -> &mut Option<U>. Thet get_or_insert is called on a new option that is dropped after creation, and the inner.max_priority_fee_per_gas is left unchanged

Changing the code to this updates the value, and tests run correctly.

this looks good, yes. the assignment is what was missing

@0xMelkor
Copy link
Contributor Author

0xMelkor commented Jan 5, 2023

this looks good, yes. the assignment is what was missing

I pushed an update.

CI is green again! 🎉

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

awesome, thanks!

@gakonst gakonst merged commit 5917e84 into gakonst:master Jan 6, 2023
@0xMelkor 0xMelkor deleted the ci-fix-nonce-manager-test branch January 7, 2023 00:00
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.

4 participants