Skip to content

dtl: handle errors in context api#2141

Merged
tynes merged 1 commit intodevelopfrom
fix/dtl-context-api-error
Feb 5, 2022
Merged

dtl: handle errors in context api#2141
tynes merged 1 commit intodevelopfrom
fix/dtl-context-api-error

Conversation

@tynes
Copy link
Contributor

@tynes tynes commented Feb 4, 2022

Description

Both GET /eth/context/latest and GET /eth/context/blocknumber/:number
rely on fetching a remote block to pull the timestamp, blocknumber and
blockhash from. If the fetched block is not found, then properties on
null will be attempted to be accessed which will result in a runtime
error. This commit adds handling for this case to prevent that kind of
bug.

Both `GET /eth/context/latest` and `GET /eth/context/blocknumber/:number`
rely on fetching a remote block to pull the timestamp, blocknumber and
blockhash from. If the fetched block is not found, then properties on
`null` will be attempted to be accessed which will result in a runtime
error. This commit adds handling for this case to prevent that kind of
bug.
@changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2022

🦋 Changeset detected

Latest commit: 8f72064

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/data-transport-layer 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

const blockNumber = Math.max(0, tip - this.options.confirmations)

const block = await this.state.l1RpcProvider.getBlock(blockNumber)
if (block === null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this results in an error either way, I think it's good to be explicit

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2022

Codecov Report

Merging #2141 (8f72064) into develop (849ec8c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2141   +/-   ##
========================================
  Coverage    72.96%   72.96%           
========================================
  Files           85       85           
  Lines         2918     2918           
  Branches       496      496           
========================================
  Hits          2129     2129           
  Misses         789      789           
Flag Coverage Δ
batch-submitter 62.63% <ø> (ø)
contracts 90.48% <ø> (ø)
core-utils 59.94% <ø> (ø)
data-transport-layer 37.74% <ø> (ø)
message-relayer 70.86% <ø> (ø)
sdk 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 849ec8c...8f72064. Read the comment docs.

@tynes tynes merged commit a22b4ed into develop Feb 5, 2022
@tynes tynes deleted the fix/dtl-context-api-error branch February 5, 2022 00:14
theochap added a commit that referenced this pull request Dec 10, 2025
## Description

This PR fixes some of the outstanding todos in kona's codebase. In
particular:

- Promotes the dial period to CLI argument for p2p. Also does a small
refactor to the way the connection gater is configured
- Open an issue to phase out the `sync-req-resp` optimism protocol
#2141. Removed the todo to promote the configuration to CLI since the
node would not sync properly if the flag is not set, and this protocol
will eventually get phased out.

Progress towards #2119
theochap added a commit that referenced this pull request Jan 14, 2026
## Description

This PR fixes some of the outstanding todos in kona's codebase. In
particular:

- Promotes the dial period to CLI argument for p2p. Also does a small
refactor to the way the connection gater is configured
- Open an issue to phase out the `sync-req-resp` optimism protocol
#2141. Removed the todo to promote the configuration to CLI since the
node would not sync properly if the flag is not set, and this protocol
will eventually get phased out.

Progress towards #2119
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants