feat[dtl]: add node env by annieke · Pull Request #735 · ethereum-optimism/optimism

annieke · 2021-05-03T20:32:47Z

Description
Adds environment and network variables to the DTL so we only conditionally initialize Sentry and monitors.

Additional context
Also includes a minor refactor of base-service and changes a verbose log to debug.

Metadata

Distinguish development, test, and prod environment for services #491

changeset-bot · 2021-05-03T20:32:49Z

🦋 Changeset detected

Latest commit: 785067d

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

This PR includes changesets to release 4 packages

Name	Type
@eth-optimism/common-ts	Patch
@eth-optimism/data-transport-layer	Patch
@eth-optimism/batch-submitter	Patch
@eth-optimism/message-relayer	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

packages/common-ts/src/base-service.ts

packages/data-transport-layer/src/services/server/service.ts

packages/data-transport-layer/src/services/main/service.ts

tynes · 2021-05-04T22:49:45Z

packages/data-transport-layer/src/services/run.ts

Is the purpose of this to make it undefined if it is not one of the 3?

yes and to make compiler pass with the stricter typing haha #735 (comment)

Maybe this should actually be chainid?

We use chain id in the other services to denote which network they run on so I do think that we could replace network name with chain id. This will be useful because all networks have network ids and not all networks we may deploy on are named yet so we won't have to make any code changes if we use chain id

tynes · 2021-05-04T22:51:53Z

packages/data-transport-layer/src/services/run.ts

You'll want to parse it as config.ufloat('sentry-trace-rate', 0.05) instead of uint
https://github.com/bcoin-org/bcfg/blob/1216c775832c2e138ae6451912a27ba496dc4386/lib/config.js#L433

good catch, fixed!

snario · 2021-05-05T00:19:24Z

packages/data-transport-layer/src/services/run.ts

Usually enums of string are unnecessary; you can just use string literals https://stackoverflow.com/questions/49761972/difference-between-string-enums-and-string-literal-types-in-ts

@snario trying string literals and it doesn't solve the not assignable from string when reading from config.str :(

I think we should apply any forceful typing at the lowest level as possible, if it means using as in this case so be it, but I still think using string as opposed to enums is preferable

annieke · 2021-05-05T16:20:24Z

fixing lint failure here: #765

smartcontracts · 2021-05-05T22:16:39Z

packages/data-transport-layer/src/services/server/service.ts

+    this.state.app.use(cors())
+    this._registerAllRoutes()
+    // Error handling needs to be initialized last
+    if (this.options.ethNetworkName)


Minor nit on using curly braces here and above. I think it's useful to be able to delineate where an if statement applies and makes it easier to add new things to the interior of the statement if necessary. Otherwise LGTM.

codecov-commenter · 2021-05-05T22:29:06Z

Codecov Report

Merging #735 (785067d) into master (34ab776) will increase coverage by 0.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #735      +/-   ##
==========================================
+ Coverage   77.53%   77.78%   +0.24%     
==========================================
  Files          48       48              
  Lines        1892     1886       -6     
  Branches      297      297              
==========================================
  Hits         1467     1467              
+ Misses        425      419       -6

Impacted Files	Coverage Δ
...imistic-ethereum/libraries/trie/Lib_MerkleTrie.sol	`74.88% <0.00%> (+1.01%)`	⬆️
...c-ethereum/libraries/trie/Lib_SecureMerkleTrie.sol	`78.57% <0.00%> (+13.86%)`	⬆️

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 34ab776...785067d. Read the comment docs.

packages/data-transport-layer/src/services/server/service.ts

* move verbose log to debug * feat: add node env to dtl * changeset and new line * conditional sentry and metrics initialization

* feat: add cgt to opcmv2 * feat: add cgt to opcmv2 * fix: change semver * feat: add cgt to opcmv2 * feat: add cgt to opcmv2 * fix: change semver * refactor: change disputeGameConfigs order in _loadFullConfig * fix: comment slash rules * feat: add opcmv2-cgt matrix in CI * fix: enable cgt overrides on OPCMv2 (#724) * fix: check CGT is not enabled before enabling it * fix: enable CGT overrides * fix: add extra instruction overrides for useCustomGasToken on OPCMv2 (#725) * chore: link TODO to issue (#727) * fix: opcm2 revert upgrade cgt (#728) * fix: add check enabled CGT during upgrades * test: set CGT false by default during forked tests * chore: expand comment on OPCMv2 regarding CGT * chore: pre-pr ready * refactor: remove unnecessary check for CGT feature (#735) * fix: cgt upgrade in opcm v2 and semver (#738) * fix: Full checkout for FFI build (#18527) * chore(op-acceptance-tests): delete very old logs (#18529) * chore(op-acceptance-tests): op-acceptor v3.8.0 (#18530) * jovian: remove feature toggles (#17978) * jovian: remove feature toggles scope is now fixed * Updated op-geth to v1.101604.0-synctest.0.0.20251120150812-e50f80a16afc * lint * just update-op-geth f48f382 * Use Jovian in test error message * just update-op-geth ba6bdcfef42341fe2b5ce124c31ff2d6b264e9e4 * chore(ai-eng): add ReinitializableBase test to exclusion list (#18531) * feat: have OPCM upgrade allowances be upgrade specific (#18462) * feat: have OPCM upgrade allowances be upgrade specific Updates the OPCMv2 check for allowed extra instructions to be specific to releases. When release versions are bumped, the allowances become automatically invalid and would reveal anywhere in the codebase where the allowance is being used. * fix: semver lock * fix: emit instruction key in error * fix: better semantics * docs: proper OPCM versioning policy * fix: final version tweaks * fix(op-acceptance-test): flake-shake; empty slack notifications. (#18542) * feat: add cgt to opcmv2 * feat: add cgt to opcmv2 * fix: change semver * feat: add cgt to opcmv2 * feat: add cgt to opcmv2 * fix: change semver * refactor: change disputeGameConfigs order in _loadFullConfig * fix: enable cgt overrides on OPCMv2 (#724) * fix: check CGT is not enabled before enabling it * fix: enable CGT overrides * fix: opcm2 revert upgrade cgt (#728) * fix: add check enabled CGT during upgrades * test: set CGT false by default during forked tests * chore: expand comment on OPCMv2 regarding CGT * fix: cgt upgrade in opcm v2 and semver (#738) * fix: merge conflicts * fix: pre-pr * fix: add missing isMatchingInstruction for cgt in false * refactor: opcm2 extra instruction keymatch (#747) * refactor: add helper function to match instruction by key only * chore: pre-pr ready * test: refactor key matching test * test: add test suite for IsMatchingKey helper function * chore: pre-pr ready * fix: opcmv2 semver --------- Co-authored-by: Flux <175354924+0xiamflux@users.noreply.github.com> Co-authored-by: Ján Jakub Naništa <jan.jakub.nanista@gmail.com> Co-authored-by: Stefano Charissis <stefano@oplabs.co> Co-authored-by: George Knee <georgeknee@googlemail.com> Co-authored-by: Ariel Diaz <65925295+aliersh@users.noreply.github.com> Co-authored-by: smartcontracts <14298799+smartcontracts@users.noreply.github.com>

annieke requested a review from snario May 3, 2021 20:34

tynes reviewed May 3, 2021

View reviewed changes

packages/common-ts/src/base-service.ts Outdated Show resolved Hide resolved

tynes reviewed May 4, 2021

View reviewed changes

packages/data-transport-layer/src/services/server/service.ts Outdated Show resolved Hide resolved

tynes reviewed May 4, 2021

View reviewed changes

packages/data-transport-layer/src/services/server/service.ts Outdated Show resolved Hide resolved

tynes reviewed May 4, 2021

View reviewed changes

packages/data-transport-layer/src/services/server/service.ts Outdated Show resolved Hide resolved

annieke requested a review from tynes May 4, 2021 16:07

snario reviewed May 4, 2021

View reviewed changes

packages/data-transport-layer/src/services/main/service.ts Outdated Show resolved Hide resolved

annieke requested a review from snario May 4, 2021 20:07

tynes reviewed May 4, 2021

View reviewed changes

annieke requested a review from tynes May 4, 2021 23:00

snario reviewed May 5, 2021

View reviewed changes

annieke requested a review from snario May 5, 2021 00:34

annieke force-pushed the feat/dtl-node-env branch from 65c89f4 to 310b644 Compare May 5, 2021 16:08

annieke added 8 commits May 5, 2021 10:32

move verbose log to debug

d6613e9

feat: add node env to dtl

2d88a84

changeset and new line

b2c5262

conditional sentry and metrics initialization

76ed970

remove options log

0679b6e

address comments

c298175

change to ufloat

263a2e2

enum to string

a42cf94

annieke force-pushed the feat/dtl-node-env branch from 310b644 to a42cf94 Compare May 5, 2021 17:32

smartcontracts reviewed May 5, 2021

View reviewed changes

brackets around if

27dec7d

annieke requested a review from smartcontracts May 5, 2021 22:21

smartcontracts reviewed May 6, 2021

View reviewed changes

packages/data-transport-layer/src/services/server/service.ts Outdated Show resolved Hide resolved

detailed comment for errors

785067d

annieke requested a review from smartcontracts May 6, 2021 16:24

smartcontracts approved these changes May 6, 2021

View reviewed changes

annieke merged commit 575bcf6 into master May 6, 2021

annieke deleted the feat/dtl-node-env branch May 6, 2021 16:43

InoMurko pushed a commit to omgnetwork/optimism that referenced this pull request May 25, 2021

feat[dtl]: add node env (ethereum-optimism#735)

17d2d65

* move verbose log to debug * feat: add node env to dtl * changeset and new line * conditional sentry and metrics initialization

OptimismBot pushed a commit that referenced this pull request Dec 8, 2025

refactor: remove unnecessary check for CGT feature (#735)

493b1f6

theochap pushed a commit that referenced this pull request Dec 10, 2025

feat: remove thiserror (#735)

89a69a4

Conversation

annieke commented May 3, 2021

Uh oh!

changeset-bot bot commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annieke commented May 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented May 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

changeset-bot bot commented May 3, 2021 •

edited

Loading

codecov-commenter commented May 5, 2021 •

edited

Loading