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

feat: added async retryer for currentL2BlockNumber in case the contra… #425

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

Conversation

jonesho
Copy link
Contributor

@jonesho jonesho commented Dec 11, 2024

…ct variable is not initialized yet

This PR implements issue(s) #317

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 Dec 11, 2024
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 11, 2024 09:48 — with GitHub Actions Inactive
@codecov-commenter
Copy link

codecov-commenter commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.14%. Comparing base (c2272f6) to head (bc65df5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #425      +/-   ##
============================================
- Coverage     70.17%   70.14%   -0.04%     
  Complexity     1070     1070              
============================================
  Files           306      306              
  Lines         12337    12330       -7     
  Branches       1179     1181       +2     
============================================
- Hits           8658     8649       -9     
- Misses         3200     3201       +1     
- Partials        479      480       +1     
Flag Coverage Δ *Carryforward flag
hardhat 98.70% <ø> (ø) Carriedforward from 93af0f5
kotlin 67.82% <100.00%> (-0.04%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...kevm/coordinator/app/LastFinalizedBlockProvider.kt 96.55% <100.00%> (-0.60%) ⬇️
...reum/gaspricing/staticcap/FeeHistoryFetcherImpl.kt 84.61% <ø> (-5.96%) ⬇️

... and 1 file with indirect coverage changes

backoffDelay = pollingInterval
) {
SafeFuture.of(lineaRollupSmartContractWeb3jClient.currentL2BlockNumber().sendAsync())
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add retry here to wait until currentL2BlockNumber on L1 is present, otherwise will get the following error:

time=2024-12-12T06:25:21,230Z level=FATAL message=org.web3j.tx.exceptions.ContractCallException: Empty value (0x) returned from contract | logger=CoordinatorAppCli thread=main | java.util.concurrent.ExecutionException: org.web3j.tx.exceptions.ContractCallException: Empty value (0x) returned from contract
2024-12-12 14:25:21     at java.base/java.util.concurrent.CompletableFuture.reportGet(Unknown Source)
2024-12-12 14:25:21     at java.base/java.util.concurrent.CompletableFuture.get(Unknown Source)
2024-12-12 14:25:21     at net.consensys.zkevm.coordinator.app.L1DependentApp.<init>(L1DependentApp.kt:308)
...

vertx,
backoffDelay = config.pollingInterval
) {
SafeFuture.of(lineaRollup.currentL2BlockNumber().sendAsync())
Copy link
Contributor Author

@jonesho jonesho Dec 12, 2024

Choose a reason for hiding this comment

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

Add retry here to wait until currentL2BlockNumber on L1 is present to avoid printing the "Empty value (0x) returned from contract" warning log message.

@@ -31,7 +31,7 @@ class FeeHistoryFetcherImpl(
}

private var cacheIsValidForBlockNumber: BigInteger = BigInteger.ZERO
private var feesCache: FeeHistory = getRecentFees().get()
private lateinit var feesCache: FeeHistory
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid calling getRecentFees().get() immediately on coordinator start-up (i.e. when L1 current ethBlockNumber is zero) which would lead to get(...) must not be null NPE

@jonesho jonesho requested a review from Filter94 December 12, 2024 19:17
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 13, 2024 04:28 — with GitHub Actions Inactive
@jonesho jonesho force-pushed the 317-improve-handling-of-errors-in-coordinator branch from 45da1fe to 93af0f5 Compare December 13, 2024 17:55
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 13, 2024 17:57 — with GitHub Actions Inactive
@jonesho jonesho temporarily deployed to docker-build-and-e2e December 14, 2024 17:57 — with GitHub Actions Inactive
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.

2 participants