Update sidecar to connect v2 #2458
Conversation
WalkthroughThe changes involve modifications to several configuration files to transition from a service named "slinky" to a new service called "connect." The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
protocol/docker-compose.yml (1)
Line range hint
1-129: Summary of changes to docker-compose.ymlThis update primarily focuses on transitioning from the
slinkysidecar to theconnectsidecar:
- The
slinky0service has been replaced withconnect0, using a new Docker image and updated configuration.- Environment variables and volume paths have been adjusted to reflect this change.
- The
dydxprotocold0service now has a different log level (info) compared to other validators (error), which is clearly commented.These changes appear to be part of a planned upgrade to the sidecar component. While the changes seem consistent, it's important to ensure that:
- All necessary configuration files are in place for the new
connectsidecar.- Any breaking changes or migration steps are documented and addressed.
- The impact of these changes on the overall system is understood and tested.
Consider updating the project documentation to reflect these changes, including any necessary steps for developers to update their local environments or for operators to migrate existing deployments.
protocol/testing/testnet-local/local.sh (1)
170-175: LGTM: Function updated to use new connect address.The
use_connectfunction has been successfully implemented, replacing the previoususe_slinkyfunction. The oracle address has been updated to 'connect0:8080', which is in line with the PR objective of updating the sidecar to connect v2.Consider using the
CONFIG_FOLDERvariable consistently throughout the function. On line 174,$VAL_CONFIG_DIRis used instead of$CONFIG_FOLDER. While this doesn't affect functionality, using consistent variable names improves readability.- dasel put -t string -f "$VAL_CONFIG_DIR"/app.toml 'oracle.oracle_address' -v 'connect0:8080' + dasel put -t string -f "$CONFIG_FOLDER"/app.toml 'oracle.oracle_address' -v 'connect0:8080'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- protocol/contrib/prometheus/prometheus.yml (1 hunks)
- protocol/docker-compose.yml (1 hunks)
- protocol/testing/testnet-local/local.sh (2 hunks)
🔇 Additional comments (5)
protocol/contrib/prometheus/prometheus.yml (1)
7-7: LGTM. Verify related configurations and update documentation.The change from "slinky0:8002" to "connect0:8002" aligns with the PR objective of updating the sidecar to connect v2. This modification appears to be part of a broader update in the system architecture or naming convention.
To ensure consistency across the project, please:
- Verify if similar changes are needed in other configuration files or services.
- Update relevant documentation to reflect this architectural shift from "slinky" to "connect".
- Confirm that the new endpoint (connect0:8002) is functional and accessible in the docker-compose environment.
Run the following script to check for other occurrences of "slinky" that might need updating:
Could you please provide information on how this change has been tested to ensure the new endpoint is working correctly?
protocol/docker-compose.yml (2)
Line range hint
10-12: LGTM: Log level change for dydxprotocold0The change to set the log level to "info" for
dydxprotocold0is well-documented with the added comment. This differentiation from other validators using the "error" log level is clear and intentional.
118-129: Major update: Transition from slinky to connect sidecarThe replacement of the
slinky0service withconnect0represents a significant change:
- The image has been updated to
ghcr.io/skip-mev/connect-sidecar:v2.1.0.- The entrypoint now uses the
connectcommand instead ofslinky.- Environment variable prefixes have changed from
SLINKY_CONFIG_PROVIDERS_toCONNECT_CONFIG_PROVIDERS_.- The volume path has been updated from
./contrib/slinkyto./contrib/connect.These changes appear consistent with the transition from
slinkytoconnect. However, to ensure a smooth transition:
- Has the
./contrib/connectdirectory been properly set up with the necessary configuration files, especially theoracle.jsonfile referenced in the entrypoint command?- Are there any breaking changes or migration steps required when moving from
slinkytoconnectthat should be documented?- Have the new command-line parameters in the entrypoint (e.g.,
--marketmap-provider dydx_migration_api) been verified for correctness?To confirm the setup, please run the following commands:
protocol/testing/testnet-local/local.sh (2)
110-111: LGTM: Function call updated to reflect new naming.The change from
use_slinkytouse_connectis consistent with the PR objective of updating the sidecar to connect v2. This update appears to be intentional and aligns with the changes described in the AI-generated summary.
Line range hint
1-200: Summary of changes and recommendationsThe changes in this file are consistent with the PR objective of updating the sidecar to connect v2. Key modifications include:
- Renaming
use_slinkytouse_connectand updating the oracle address.- Removing TEST_ACCOUNTS from the
edit_genesisfunction call.Recommendations:
- Ensure consistent use of the
CONFIG_FOLDERvariable in theuse_connectfunction.- Provide clarification on the impact of removing TEST_ACCOUNTS from the genesis setup process.
Overall, the changes appear to be well-implemented, but addressing the above points will improve code consistency and provide better context for the modifications made.
| @@ -107,7 +107,7 @@ create_validators() { | |||
| cat <<<"$new_file" >"$VAL_CONFIG_DIR"/node_key.json | |||
|
|
|||
| edit_config "$VAL_CONFIG_DIR" | |||
| use_slinky "$VAL_CONFIG_DIR" | |||
| use_connect "$VAL_CONFIG_DIR" | |||
|
|
|||
| # Using "*" as a subscript results in a single arg: "dydx1... dydx1... dydx1..." | |||
| # Using "@" as a subscript results in separate args: "dydx1..." "dydx1..." "dydx1..." | |||
There was a problem hiding this comment.
💡 Codebase verification
TEST_ACCOUNTS are still referenced in multiple scripts.
Removing "${TEST_ACCOUNTS[*]}" from the edit_genesis function in local.sh may affect other environments that rely on TEST_ACCOUNTS. Please ensure that this change is consistently applied across all relevant scripts or provide clarification on how TEST_ACCOUNTS is being managed moving forward.
🔗 Analysis chain
Verify impact of removing TEST_ACCOUNTS from genesis setup.
The removal of "${TEST_ACCOUNTS[*]}" from the edit_genesis function call aligns with the changes described in the AI-generated summary. This modification appears to be part of simplifying the genesis setup process.
Could you please clarify the impact of removing the TEST_ACCOUNTS from the genesis setup? Are these accounts now being initialized through a different process, or are they no longer needed in the new setup?
To verify the changes in account initialization, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any references to TEST_ACCOUNTS in the genesis setup process
# Search for TEST_ACCOUNTS usage in the file
echo "Occurrences of TEST_ACCOUNTS in local.sh:"
grep -n "TEST_ACCOUNTS" protocol/testing/testnet-local/local.sh
# Check if TEST_ACCOUNTS are used in other parts of the codebase
echo "Occurrences of TEST_ACCOUNTS in other files:"
rg "TEST_ACCOUNTS" --type sh
Length of output: 2297
Changelist
With the protocol update to use the connect v2 package, we also need to update the sidecar because endpoints have changed.
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
connect0to replaceslinky0in the multi-service setup.connectservice.Bug Fixes
Chores