Upgrade connect dependency to allow market authorities to remove markets #2615
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
protocol/go.mod (1)
Line range hint
466-474: Track TODO items for future cleanupThere are several TODO comments indicating temporary replacements that need to be addressed:
- DEC-2209: Waiting for latest signing mode fixes to be released
- [Bug]: Builds fail due to dependency conflict with cosmossdk.io/core v0.11.0 -> v0.12.0 comet.BlockInfo -> comet.Info symbol rename cosmos/rosetta#76: Version mismatch in Rosetta dependency
Would you like me to create GitHub issues to track these TODOs?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
protocol/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
protocol/Dockerfile(3 hunks)protocol/go.mod(21 hunks)
🔇 Additional comments (2)
protocol/Dockerfile (1)
3-3: Verify the Docker image digest
Let's verify the authenticity of the Docker image digest.
protocol/go.mod (1)
72-72: Dependency upgrade aligns with PR objectives
The upgrade of skip-mev/connect/v2 to v2.2.1 supports the PR's objective of allowing market authorities to remove markets.
| # `docker buildx imagetools inspect golang:1.23.1-alpine` | ||
| ARG GOLANG_1_23_ALPINE_DIGEST="ac67716dd016429be8d4c2c53a248d7bcdf06d34127d3dc451bda6aa5a87bc06" |
There was a problem hiding this comment.
Critical: Invalid Go version specified
The Dockerfile references golang:1.23.1-alpine, but this is an invalid Go version. Go follows semantic versioning, and version 1.23 does not exist. The latest stable version is in the 1.22.x series.
Apply this diff to use the correct Go version:
-# `docker buildx imagetools inspect golang:1.23.1-alpine`
-ARG GOLANG_1_23_ALPINE_DIGEST="ac67716dd016429be8d4c2c53a248d7bcdf06d34127d3dc451bda6aa5a87bc06"
+# `docker buildx imagetools inspect golang:1.22.2-alpine`
+ARG GOLANG_1_22_ALPINE_DIGEST="0da8b5a5f9e5c0403c67b259a0e3d19b0cb3f8dd8a1e7fa2a1692d3ddb18e3da"And update the corresponding FROM statements accordingly.
Also applies to: 12-12, 49-49
| go 1.23.1 | ||
|
|
||
| toolchain go1.23.3 |
There was a problem hiding this comment.
Invalid Go version specified
The Go version 1.23.1 and toolchain version 1.23.3 are invalid. Go uses semantic versioning, and the latest stable version is in the 1.22.x series.
Apply this diff to fix the Go version:
-go 1.23.1
+go 1.22.2
-toolchain go1.23.3
+toolchain go1.22.2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go 1.23.1 | |
| toolchain go1.23.3 | |
| go 1.22.2 | |
| toolchain go1.22.2 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
protocol/daemons/pricefeed/client/price_fetcher/mutable_state_test.go (1)
74-74: Improved pointer comparison for better state isolationThe change from value comparison to pointer comparison (
&marketExponents, &pf.mutableState.marketExponents) provides stronger guarantees that the returned market exponents are completely independent copies. This is crucial for preventing shared mutable state, especially important in the context of market management operations.This pattern of strict state isolation through deep copies should be maintained consistently across the codebase, particularly for market-related operations to prevent potential data corruption during market deletions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
protocol/daemons/pricefeed/client/price_fetcher/mutable_state_test.go(2 hunks)protocol/daemons/pricefeed/client/types/price_feed_mutable_market_configs_test.go(1 hunks)
🔇 Additional comments (1)
protocol/daemons/pricefeed/client/price_fetcher/mutable_state_test.go (1)
4-5: LGTM: Clean import addition
The testing package import is properly formatted and positioned.
| @@ -155,7 +155,7 @@ func TestGetMarketConfigCopies(t *testing.T) { | |||
| require.Error(t, err, tc.ExpectedError.Error()) | |||
| } else { | |||
| // Validate that this method returns a copy and not the original. | |||
| require.NotSame(t, tc.Expected, actual) | |||
| require.NotSame(t, &tc.Expected, &actual) | |||
There was a problem hiding this comment.
Fix incorrect pointer comparison in slice copy test.
The current assertion require.NotSame(t, &tc.Expected, &actual) only verifies that the slice headers have different addresses, which is always true for different slices. This doesn't guarantee that the individual market configs within the slice are actually copies.
To properly test that each market config is a copy, replace the assertion with:
- require.NotSame(t, &tc.Expected, &actual)
+ require.Equal(t, len(tc.Expected), len(actual))
+ for i := range tc.Expected {
+ require.NotSame(t, tc.Expected[i], actual[i])
+ require.Equal(t, tc.Expected[i], actual[i])
+ }This change will:
- Verify that both slices have the same length
- Check that each market config in the slice is a different instance (NotSame)
- Verify that the copied market configs have the same values (Equal)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require.NotSame(t, &tc.Expected, &actual) | |
| require.Equal(t, len(tc.Expected), len(actual)) | |
| for i := range tc.Expected { | |
| require.NotSame(t, tc.Expected[i], actual[i]) | |
| require.Equal(t, tc.Expected[i], actual[i]) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
protocol/daemons/pricefeed/client/price_fetcher/mutable_state_test.go(4 hunks)protocol/daemons/pricefeed/client/price_fetcher/price_fetcher_test.go(4 hunks)protocol/daemons/pricefeed/client/types/mutable_exchange_market_config_test.go(2 hunks)protocol/daemons/pricefeed/client/types/mutable_market_config_test.go(2 hunks)protocol/daemons/pricefeed/client/types/price_feed_mutable_market_configs_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- protocol/daemons/pricefeed/client/price_fetcher/mutable_state_test.go
- protocol/daemons/pricefeed/client/types/price_feed_mutable_market_configs_test.go
🔇 Additional comments (5)
protocol/daemons/pricefeed/client/types/mutable_exchange_market_config_test.go (2)
5-6: LGTM: Clean import organization
The removal of duplicate import and proper spacing follows Go style conventions.
39-41: LGTM: Improved deep copy verification
The use of NotSame to compare pointer addresses, combined with Equal to verify content, provides a more accurate test of deep copy behavior. This ensures that:
- The copy is a distinct object in memory (different pointer)
- The copied content is identical (equal values)
protocol/daemons/pricefeed/client/price_fetcher/price_fetcher_test.go (3)
7-8: LGTM! Import organization looks good.
The reorganization of imports improves readability by properly grouping the daemon types import.
214-214: LGTM! Improved pointer comparison assertions.
The changes correctly test that getTaskLoopDefinition returns a new copy of the mutable config rather than the same instance, which is crucial for thread safety in concurrent operations.
Also applies to: 224-224
252-252: LGTM! Consistent pointer comparison assertion.
The change aligns with the previous test function's assertions, ensuring proper pointer comparison for thread safety validation in the multi-market exchange scenario.
| @@ -18,7 +19,7 @@ func TestCopy(t *testing.T) { | |||
|
|
|||
| mmcCopy := mmc.Copy() | |||
|
|
|||
| require.NotSame(t, mmc, mmcCopy) | |||
| require.NotSame(t, &mmc, &mmcCopy) | |||
There was a problem hiding this comment.
Fix incorrect pointer comparison in TestCopy
The current change introduces a bug in the pointer comparison. Since mmc is already a pointer (*types.MutableMarketConfig), taking its address with & is incorrect and will compare the addresses of the pointer variables rather than the actual object addresses.
Apply this fix:
- require.NotSame(t, &mmc, &mmcCopy)
+ require.NotSame(t, mmc, mmcCopy)This ensures we're correctly verifying that the Copy() method creates a new object with different memory address while maintaining the same values.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require.NotSame(t, &mmc, &mmcCopy) | |
| require.NotSame(t, mmc, mmcCopy) |
chenyaoy
deleted the
chenyao/allow-market-authority-to-remove-markets
branch
Changelist
Test Plan
Tested on localnet that:
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
Bug Fixes
Documentation