Skip to content

integration-tests: fee too low/too high#1736

Merged
tynes merged 3 commits intodevelopfrom
fix/ops-config
Dec 6, 2021
Merged

integration-tests: fee too low/too high#1736
tynes merged 3 commits intodevelopfrom
fix/ops-config

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented Nov 10, 2021

Description
Enforce fees in the integration tests so that
tests can be written to cover the fee rejection
logic. This was previously unit tested in geth
without end to end test coverage. Projects
depend on the error strings returned, so it is
important to have test coverage over them.

Previously the owner of the gas price oracle was set as
the same wallet used in the l2Wallet in the
integration tests. This would allow it to send
0 gas price transactions, as the sequencer will not
charge for transactions from the owner of the gpo.
The sequencer also updates the cached values
from the contract whenever the owner of the gpo
sends a transaction, which results in extra
logs and makes the test environment less realistic.

Updates the integration tests to work when fees
are enforced by the sequencer. This allows for
integration test coverage of the cases where transactions
are rejected when fees are too low or too high.

@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2021

🦋 Changeset detected

Latest commit: b1fa3f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/integration-tests Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@tynes tynes requested review from annieke and mslipper November 10, 2021 07:13
@github-actions github-actions bot added 2-reviewers A-integration Area: integration tests A-ops Area: ops labels Nov 10, 2021
@tynes tynes requested a review from smartcontracts November 10, 2021 07:14
@tynes tynes changed the title Fix/ops config integration-tests: fee too low/too high Nov 10, 2021
@tynes
Copy link
Contributor Author

tynes commented Nov 10, 2021

Enforcing fees may have downstream effects of impacting users that relied on gas price 0 transactions. There isn't really a good way to test these cases in the integration tests without turning on fee enforcement. Users can unset that config at runtime if they so choose

@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2021

Codecov Report

Merging #1736 (41189ec) into develop (e0589e0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1736   +/-   ##
========================================
  Coverage    71.90%   71.90%           
========================================
  Files           69       69           
  Lines         2310     2310           
  Branches       345      345           
========================================
  Hits          1661     1661           
  Misses         649      649           
Flag Coverage Δ
batch-submitter 62.07% <ø> (ø)
contracts 87.96% <ø> (ø)
core-utils 56.53% <ø> (ø)
data-transport-layer 38.23% <ø> (ø)
message-relayer 70.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0589e0...41189ec. Read the comment docs.

@github-actions github-actions bot added A-op-batcher Area: op-batcher M-ci Meta: ci related work M-contracts A-pkg-core-utils Area: packages/core-utils A-cannon Area: cannon labels Nov 10, 2021
Base automatically changed from regenesis/0.5.0 to develop November 10, 2021 22:08
@github-actions github-actions bot removed M-ci Meta: ci related work M-dtl A-cannon Area: cannon labels Nov 23, 2021
@github-actions github-actions bot removed A-op-batcher Area: op-batcher M-contracts A-pkg-core-utils Area: packages/core-utils labels Nov 23, 2021
tynes added 3 commits December 6, 2021 13:14
Enforce fees in the integration tests so that
tests can be written to cover the fee rejection
logic. This was previously unit tested in geth
without end to end test coverage. Projects
depend on the error strings returned, so it is
important to have test coverage over them.

Previously the owner of the gas price oracle was set as
the same wallet used in the `l2Wallet` in the
integration tests. This would allow it to send
0 gas price transactions, as the sequencer will not
charge for transactions from the owner of the gpo.
The sequencer also updates the cached values
from the contract whenever the owner of the gpo
sends a transaction, which results in extra
logs and makes the test environment less realistic.
Updates the integration tests to work when fees
are enforced by the sequencer. This allows for
integration test coverage of the cases where transactions
are rejected when fees are too low or too high.
@tynes
Copy link
Contributor Author

tynes commented Dec 6, 2021

We have gotten the go ahead with synthetix to merge this PR

@tynes tynes merged commit b7e9d10 into develop Dec 6, 2021
@tynes tynes deleted the fix/ops-config branch December 6, 2021 22:12
theochap pushed a commit that referenced this pull request Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-integration Area: integration tests A-ops Area: ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants