Skip to content
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.

Sign up for GitHub

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

Jump to bottom

e2e: Migrate dynamic fees test from kurtosis #1792

Merged
merged 6 commits into from
Sep 22, 2023
Merged

e2e: Migrate dynamic fees test from kurtosis #1792

merged 6 commits into from
Sep 22, 2023

Conversation

marun
Copy link
Contributor

@marun marun commented Aug 2, 2023 •
edited
Loading

Why this should be merged

Fulfills one of the requirements for #1547 (migration of Kurtosis tests)

How this works

  • reimplements avalanche testing's dynamic fees test
  • enables use of a test-private network (possible now that the shared network is only 2 nodes) to avoid having other tests interfere with gas pricing

How this was tested

Ran ./scripts/test.e2e.sh

TODO

@marun marun added the testing This primarily focuses on testing label Aug 2, 2023
@marun marun mentioned this pull request Jul 27, 2023
Closed
16 tasks
@marun marun force-pushed the e2e-dynamic-fees branch 7 times, most recently from 48cabb2 to f3ba496 Compare August 20, 2023 21:54
@marun marun linked an issue Aug 23, 2023 that may be closed by this pull request
Migrate Kurtosis Tests #1547
Closed
16 tasks
@marun marun self-assigned this Aug 23, 2023
@marun marun marked this pull request as ready for review August 25, 2023 21:00
@marun marun requested review from abi87 and gyuho as code owners August 25, 2023 21:00
@marun marun force-pushed the e2e-dynamic-fees branch 2 times, most recently from 3266c3f to 2c9c414 Compare August 27, 2023 19:57
@marun marun marked this pull request as draft August 30, 2023 16:36
@marun marun force-pushed the e2e-dynamic-fees branch 4 times, most recently from d63238f to ec8341b Compare August 31, 2023 18:39
@marun marun mentioned this pull request Aug 31, 2023
Merged
2 tasks
danlaine
danlaine reviewed Sep 12, 2023
View reviewed changes
Copy link

@danlaine danlaine left a comment

Choose a reason for hiding this comment

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

1 nit otherwise looks OK

tests/e2e/c/hashing_contract.go
// See the file LICENSE for licensing terms.

// AUTOMATICALLY GENERATED. DO NOT EDIT!
// Generated from hashing.sol by compile-contract.sh
Copy link

Choose a reason for hiding this comment

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

Can we add compile-contract.sh to this directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lifted this verbatim from avalanche-testing, and the script wasn't committed in that case either. iirc from Aaron that the script in question is only one part of a larger dependency chain and I don't think we would want to commit it here. I figure we have something that works and in the unlikely even that we ever need to modify it we can engage our resident solidity experts to do so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We supported this until go-ethereum removed their Solidity compiler package. I'm not opposed to installing solc and using that where necessary.

I don't have a strong opinion on whether it's better to install solc and compile on-demand or hardcode and am open to either.

marun
marun commented Sep 12, 2023
View reviewed changes
tests/e2e/c/dynamic_fees.go Outdated Show resolved Hide resolved
marun
marun commented Sep 12, 2023
View reviewed changes
tests/e2e/c/dynamic_fees.go
// the contract to induce a gas price increase
const largeGasLimit = uint64(8_000_000)

// TODO(marun) What is the significance of this value?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronbuchwald Any hints as to why this is an appropriate value?

@marun marun force-pushed the e2e-dynamic-fees branch 3 times, most recently from a36c30f to 1290573 Compare September 21, 2023 06:46
@marun
Copy link
Contributor Author

marun commented Sep 21, 2023

Rebased

@marun marun force-pushed the e2e-dynamic-fees branch 2 times, most recently from 07379fb to 942a886 Compare September 21, 2023 20:00
This was referenced Sep 22, 2023
Merged
Merged
hexfusion
hexfusion reviewed Sep 22, 2023
View reviewed changes
tests/e2e/c/hashing.sol Show resolved Hide resolved
tests/e2e/c/dynamic_fees.go Show resolved Hide resolved
tests/e2e/c/dynamic_fees.go Show resolved Hide resolved
tests/e2e/c/dynamic_fees.go Outdated Show resolved Hide resolved
tests/e2e/e2e.go Show resolved Hide resolved
tests/e2e/e2e.go Show resolved Hide resolved
tests/e2e/e2e.go Show resolved Hide resolved
@marun
Copy link
Contributor Author

marun commented Sep 22, 2023

Rebased

@StephenButtolph StephenButtolph added this to the v1.10.11 milestone Sep 22, 2023
@StephenButtolph StephenButtolph added this pull request to the merge queue Sep 22, 2023
Merged via the queue into dev with commit 1d45599 Sep 22, 2023
16 checks passed
@StephenButtolph StephenButtolph deleted the e2e-dynamic-fees branch September 22, 2023 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danlaine danlaine danlaine approved these changes

@hexfusion hexfusion hexfusion approved these changes

@abi87 abi87 Awaiting requested review from abi87

@gyuho gyuho Awaiting requested review from gyuho

@StephenButtolph StephenButtolph Awaiting requested review from StephenButtolph

@aaronbuchwald aaronbuchwald Awaiting requested review from aaronbuchwald

Assignees

@marun marun

Labels
testing This primarily focuses on testing
Projects
No open projects
Ski Patrol
Archived in project
Milestone
v1.10.11
Development

Successfully merging this pull request may close these issues.

Migrate Kurtosis Tests
5 participants
@marun @hexfusion @danlaine @aaronbuchwald @StephenButtolph