-
Notifications
You must be signed in to change notification settings - Fork 225
chore(proxy): consolidate dotenv examples files into single one #1800
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
base: master
Are you sure you want to change the base?
Conversation
Having multiple example files is a source of bug and confusion. its also hard to maintain. Consolidating into a single .env.example file. People should figure out how to populate it for their own network anyways. Also removed all the config values that have sane defaults, and only left the ones that are mandatory to set. This removes a lot of cognitive load on our users, and will be a helpful simplification I believe.
Very minor bug that was being misread. belive its a urfave bug but havent spent more time investigating.
The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).
|
@@ -50,7 +37,7 @@ jobs: | |||
experimental: true | |||
working_directory: api/proxy | |||
- name: Run flag test | |||
run: ${{ github.workspace }}/api/proxy/scripts/test-proxy-startup-with-env-vars.sh ${{ matrix.env-file }} | |||
run: ${{ github.workspace }}/api/proxy/scripts/test-proxy-startup-with-env-vars.sh .env.example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to take the suggestion to not use the.env file?
# | ||
# See https://layr-labs.github.io/eigenda/integration/spec/6-secure-integration.html#upgradable-quorums-and-thresholds-for-optimistic-verification | ||
# for more details. | ||
EIGENDA_PROXY_EIGENDA_V2_CERT_VERIFIER_ROUTER_OR_IMMUTABLE_VERIFIER_ADDR=0x61692e93b6B045c444e942A91EcD1527F23A3FB7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think it is better to remove both mainnet and this address, but feel free to ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you want to leave a default one, perhaps it is better to leave the one for Sepolia.
Because for me now it's good to have a mental load.
# SRS data that will be read into memory. Reading in a large amount of SRS data can cause long startup times, and since | ||
# you only actually need to read the amount of SRS data that corresponds to the size of the largest blob that will be | ||
# sent, decreasing this value is a crude sort of optimization. | ||
EIGENDA_PROXY_EIGENDA_V2_MAX_BLOB_LENGTH=1MiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have spent so much word here, maybe it's also good to add a caution to readers that this is different from the payload size because of padding and headers.
# Allowed distance (in L1 blocks) between the eigenDA cert's reference block number (RBN) | ||
# and the L1 block number at which the cert was included in the rollup's batch inbox. | ||
# A cert is considered valid if certL1InclusionBlock <= cert.RBN + rbnRecencyWindowSize, | ||
# otherwise an HTTP 418 (TEAPOT) error is returned to indicate that is should be dropped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For secure integration, for insecure integration, I don't think we have made a decision on which one is better.
- stall or
- skip
when encountering a derivation error. What is your vote on this, though this is a broader discussion.
# === V2 Configuration === | ||
|
||
# Hex-encoded signer private key for payments with EigenDA V2 disperser. | ||
EIGENDA_PROXY_EIGENDA_V2_SIGNER_PRIVATE_KEY_HEX="0000000000000000000100000000000000000000000000000000000000000000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty surprised that this private key is allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, how'd you get access to my cold storage key?!?!?
for the exact values getting set by this flag. All of those values can also be manually | ||
set via their respective flags, and take precedence over the default values set by the network flag. | ||
If all of those other flags are manually configured, the network flag may be omitted. | ||
Permitted EigenDANetwork values include %s, %s, %s, & %s.`, | ||
common.MainnetEigenDANetwork, | ||
common.HoleskyTestnetEigenDANetwork, | ||
common.HoleskyPreprodEigenDANetwork, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a thought:
right now, the eigenda_network
is a bit hard to maintain. If you want to add a network type, you have to define the name constant, and then make sure you add the correct values to each switch statement in that file.
It would be a lot easier if we just defined an EigenDANetwork
struct type, which contains all the necessary fields. Then all values for a given network would be defined together.
Additionally, if we maintained a list of all existing EigenDANetwork
types in eigenda_network.go
, we could use it for logs like this (join all the names with ,
), and then adding a new network type wouldn't entail modifying multiple files, it would all be contained in eigenda_network.go
This isn't necessarily something that needs to be done right now, or at all.
// since our code downstream doesn't work well with empty strings. | ||
// Specifically, if the env var is simply set to nothing like `EIGENDA_PROXY_STORAGE_CACHE_TARGETS=`, | ||
// it will result in an empty string being added to the slice | ||
// for some reason... seems like a bug in urfave/cli? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not a bug, at least an unpleasant user experience.
# === V2 Configuration === | ||
|
||
# Hex-encoded signer private key for payments with EigenDA V2 disperser. | ||
EIGENDA_PROXY_EIGENDA_V2_SIGNER_PRIVATE_KEY_HEX="0000000000000000000100000000000000000000000000000000000000000000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, how'd you get access to my cold storage key?!?!?
# === Storage Configuration === | ||
|
||
# The storage backends to enable. Options are [V1, V2] | ||
EIGENDA_PROXY_STORAGE_BACKENDS_TO_ENABLE=V1,V2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding additional guidance here:
The contents of this field dictate whether the structures to support the given backend are instantiated or not.
Reasons to include both V1
and V2
include:
- You're dispersing to one, but still want to support reads from the other
- You want to change which backend you are dispersing to on the fly, i.e. you're migrating from V1 to V2. So everything would be going through V1 at first, but the necessary structures would exist to switch to V2 dispersals, without requiring a restart
EIGENDA_PROXY_STORAGE_BACKENDS_TO_ENABLE=V1,V2 | ||
|
||
# Target EigenDA backend version (V1 or V2) for blob dispersals (POST routes). | ||
EIGENDA_PROXY_STORAGE_DISPERSAL_BACKEND=V2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider noting that whichever you choose, it must also be part of the EIGENDA_PROXY_STORAGE_BACKENDS_TO_ENABLE
list
api/proxy/.env.example
Outdated
# === Metrics Configuration === | ||
|
||
# Enable the metrics server, which is disabled by default. | ||
EIGENDA_PROXY_METRICS_ENABLED=true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on which merges first, you'll need to make sure to remove this, considering your PR to enable metrics by default
- Set `EIGENDA_PROXY_STORAGE_DISPERSAL_BACKEND=V2`, so that the proxy started with this config will immediately | ||
enable V2 dispersal | ||
|
||
3. **Scheduled Migration** | ||
1. **Scheduled Migration** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
improper numbering
* chore(proxy): enable metrics server by default This simplifies the .env.example config file, and makes one less configuration to search for for new users. * chore: make gen output
I merged #1801 into this by mistake... should prob revert that. Although its a small change so might be fine to just merge all of this together. |
Why are these changes needed?
Closes DAINT-649.
Having multiple example files is a source of bug and confusion. its also hard to maintain. Consolidating into a single .env.example file. People should figure out how to populate it for their own network anyways.
Also removed all the config values that have sane defaults, and only left the ones that are mandatory to set. This removes a lot of cognitive load on our users, and will be a helpful simplification I believe.
Checks