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.

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

3822 rejected transaction api service #101

Open
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

jonesho
Copy link
Contributor

@jonesho jonesho commented Sep 25, 2024

This PR implements issue(s) #

Checklist

  • I wrote new tests for my new core changes.
  • I have successfully ran tests, style checker and build against my new changes locally.
  • I have informed the team of any breaking changes if there are any.

@jonesho jonesho self-assigned this Sep 25, 2024
Copy link

github-actions bot commented Sep 25, 2024

Delta Summary - Kotlin Code Coverage

Generated on: 10/03/2024 - 08:37
Description Previous Current Delta
Coverage date: 10/03/2024 - 08:37 10/03/2024 - 08:37
Tag: 651_11157913743 651_11157913743
Line coverage: 27.1% 27.1% 0.0%
Covered lines: 25877 25877 0
Coverable lines: 95371 95371 0
Total lines: 136038 136038 0
Branch coverage: 10.4% 10.4% 0.0%
Covered branches: 5566 5566 0
Total branches: 53031 53031 0
Method coverage: Feature is only available for sponsors

@jonesho jonesho force-pushed the 3822-rejected-transaction-api-service branch from 48c374d to 2bba6d0 Compare September 26, 2024 05:36
@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.33%. Comparing base (c186d66) to head (5a3380f).

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #101      +/-   ##
============================================
- Coverage     70.38%   68.33%   -2.06%     
- Complexity     1040     1053      +13     
============================================
  Files           282      248      -34     
  Lines         11314    10398     -916     
  Branches       1082      867     -215     
============================================
- Hits           7963     7105     -858     
+ Misses         2880     2824      -56     
+ Partials        471      469       -2     
Flag Coverage Δ
hardhat ?
kotlin 68.33% <100.00%> (+0.47%) ⬆️
Files with missing lines Coverage Δ
...otlin/net/consensys/linea/metrics/MetricsFacade.kt 100.00% <100.00%> (ø)
...in/kotlin/net/consensys/zkevm/persistence/db/Db.kt 100.00% <100.00%> (ø)
...lin/net/consensys/zkevm/persistence/db/DbHelper.kt 56.25% <ø> (ø)
...nsensys/zkevm/persistence/db/PersistenceRetryer.kt 100.00% <100.00%> (ø)
...ys/zkevm/persistence/db/PostgresHelperFunctions.kt 100.00% <ø> (ø)
...t/consensys/zkevm/persistence/db/SQLQueryLogger.kt 38.88% <ø> (ø)

... and 42 files with indirect coverage changes

@jonesho jonesho force-pushed the 3822-rejected-transaction-api-service branch from 2bba6d0 to a6f95fa Compare September 26, 2024 07:32
.github/workflows/transaction-exclusion-api-testing.yml Outdated Show resolved Hide resolved
.github/workflows/transaction-exclusion-api-testing.yml Outdated Show resolved Hide resolved
import { beforeAll, describe, expect, it } from "@jest/globals";
import { TransactionExclusionClient } from "./utils/utils";

const transactionExclusionTestSuite = (title: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

As already mentioned before, this shall be an integration test inside transaction-exclusion-api:app module, not an end-to-end tests. This test is a sort of integration/contract test, not end2end.

We shall have, however, a true e2e test: send a Transaction to the sequencer that cannot be proven, and verify that it was delivered and available through linea_getTransactionExclusionStatusV1 . This will ensure that the whole e2e flow works.

Copy link
Contributor Author

@jonesho jonesho Sep 30, 2024

Choose a reason for hiding this comment

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

Yes, I'm opted for a true e2e test too, but we need to wait for the release of the linea-sequencer plugin with this PR which would support reporting rejected transactions on tx simulation, but more importantly, the fixes of some blocking issues such as: missing overflows field in the save rejected transaction request and more descriptive reason messages. I'd update the e2e tests once the release is ready from besu team

Copy link
Collaborator

Choose a reason for hiding this comment

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

As already mentioned before, this shall be an integration test inside transaction-exclusion-api:app module.

Still, the end2end test you will write does not invalidate the integration test inside transaction-exclusion-api:app they serve different purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, integration tests inside transaction-exclusion-api:app will be added

@jonesho jonesho force-pushed the 3822-rejected-transaction-api-service branch 3 times, most recently from 2309502 to e29b83a Compare October 3, 2024 07:06
@jonesho jonesho force-pushed the 3822-rejected-transaction-api-service branch 4 times, most recently from 235df48 to f1492c2 Compare October 9, 2024 13:01
import kotlin.random.Random

@ExtendWith(VertxExtension::class)
class TransactionExclusionAppTest : CleanDbTestSuiteParallel() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, I'm making this integration test to be able to run each test case in parallel with no order restriction instead of in sequential order (which was the initial design), the tradeoff is more databases to create (each per test case) and more code for initial setup in each test case.

@@ -202,9 +202,39 @@ jobs:
e2e-tests-logs-dump: true
secrets: inherit

run-e2e-tests-tx-exclusion-geth-tracing:
Copy link
Contributor Author

@jonesho jonesho Oct 9, 2024

Choose a reason for hiding this comment

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

Here, I separate the transaction exclusion e2e tests from the existing e2e tests and let them run in different runners to achieve parallelism, the reason is not to increase the total time for all e2e tests given that transaction exclusion e2e tests are not related to any of the existing e2e tests, and also given that they would use a sequencer and l2 besu node with reduced trace limits (otherwise if using the default trace limits, it would take quite some time or even timeout for the nodes to reject an over traces-limit transaction), so it'd be best to separate them from the existing e2e tests (where the nodes are shared across each e2e test)

@jonesho jonesho requested a review from jpnovais October 9, 2024 15:34
@jonesho jonesho force-pushed the 3822-rejected-transaction-api-service branch 2 times, most recently from 2d2f032 to f75cc83 Compare October 14, 2024 08:36
jonesho and others added 21 commits October 15, 2024 01:42
@jonesho jonesho force-pushed the 3822-rejected-transaction-api-service branch from d227ddf to 796aa7c Compare October 14, 2024 17:44
@jonesho jonesho deployed to docker-build-and-e2e October 14, 2024 18:26 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants