chore: Update slinky version #1768
Conversation
WalkthroughThe updates broadly refactor and enhance the Slinky protocol, focusing on Oracle initialization and Prometheus metrics management. They eliminate unused imports, clean up the configuration, enhance the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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: 4
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 (10)
- protocol/app/app.go (3 hunks)
- protocol/app/vote_extensions/extend_vote_handler.go (2 hunks)
- protocol/cmd/dydxprotocold/cmd/config.go (1 hunks)
- protocol/daemons/flags/flags.go (3 hunks)
- protocol/daemons/types/grpc_client.go (2 hunks)
- protocol/go.mod (13 hunks)
- protocol/mocks/OracleClient.go (2 hunks)
- protocol/testing/containertest/node.go (1 hunks)
- protocol/x/prices/keeper/slinky_adapter.go (1 hunks)
- protocol/x/prices/keeper/slinky_adapter_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/daemons/flags/flags.go
- protocol/testing/containertest/node.go
Additional comments not posted (5)
protocol/app/vote_extensions/extend_vote_handler.go (1)
24-32: Introduction of theNoopPriceAppliertype and its methods.The
NoopPriceAppliertype and its methodsApplyPricesFromVoteExtensionsandGetPricesForValidatorare added as part of the refactoring. It's important to ensure that these methods are correctly implemented and documented, especially since they return nil or default values. This might indicate a placeholder or minimal implementation.protocol/mocks/OracleClient.go (1)
20-55: Addition of theMarketMapmethod to the mockOracleClient.The
MarketMapmethod is correctly implemented as a mock with appropriate setup for handling variadicCallOptionparameters and returning values based on theCalledmethod. Ensure that this mock is used in unit tests to simulate the behavior of the Oracle client accurately.protocol/x/prices/keeper/slinky_adapter_test.go (1)
117-136: Addition of new test functions for currency pair management.The test functions
TestGetAllCurrencyPairsandTestGetNumCurrencyPairsare added to verify the new functionality related to currency pairs in theslinky_adapter. These tests appear to be well-structured and should effectively validate the correctness of the related functionality.protocol/x/prices/keeper/slinky_adapter.go (1)
108-149: Enhancements to currency pair management in theKeeperstruct.The new methods
GetNumCurrencyPairs,GetNumRemovedCurrencyPairs, andGetAllCurrencyPairsare added to manage currency pairs more effectively. These methods are well-implemented, utilizing efficient iteration and error handling. Ensure that they are integrated properly in the overall workflow of theKeeper.protocol/go.mod (1)
8-8: Dependency updates reviewed and approved.All the dependency updates listed are to newer versions, which typically include bug fixes, performance improvements, and security patches. It's crucial to ensure these updates do not introduce breaking changes through thorough testing.
Also applies to: 11-11, 12-12, 13-13, 14-14, 16-16, 22-22, 32-32, 35-35, 37-37, 38-38, 39-39, 49-49, 51-51, 54-54, 55-55, 58-58, 60-60, 62-62, 63-63, 65-65, 70-70, 71-71, 73-73, 74-74
protocol/app/app.go
Outdated
| // We are not using the slinky PreBlocker/PriceApplier, so there is no need to pass in PreBlocker here for | ||
| // VE handler to work properly. | ||
| // Currently the clob PreBlocker assumes that it will only be called during the normal ABCI | ||
| // PreBlocker step. Passing in the app PreBlocker here will break that assumption by causing | ||
| // the clob PreBlocker to be called unexpectedly. This to leads improperly initialized clob state | ||
| // which results in the next block being committed incorrectly. | ||
| func(_ sdk.Context, _ *abci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { | ||
| return nil, nil | ||
| }, | ||
| // TODO we can move the UpdateMarketPrices in extend vote to this in the future. | ||
| vote_extensions.NoopPriceApplier{}, |
There was a problem hiding this comment.
Consider Refactoring the Vote Extension Handler Initialization
The comment at line 1596 suggests that the current implementation of the vote_extensions.NoopPriceApplier in the vote_extensions.ExtendVoteHandler might lead to unexpected behaviors due to assumptions about when the PreBlocker is called. This could potentially lead to state inconsistencies or incorrect block commitments.
Recommendation: It would be beneficial to refactor this initialization to ensure that the PreBlocker is only invoked during the expected ABCI lifecycle phase. This might involve reworking the control flow or state management within the handler to prevent unintended side effects.
| Enabled: true, | ||
| OracleAddress: "localhost:8080", | ||
| ClientTimeout: time.Second * 2, | ||
| MetricsEnabled: false, |
There was a problem hiding this comment.
Ensure proper error handling for Oracle configuration.
The Oracle configuration in DydxAppConfig lacks error handling for potential connectivity issues with the Oracle server. Given the critical role of Oracles in blockchain applications, robust error handling and fallback mechanisms are essential.
+ // TODO: Add error handling for Oracle connectivity issues.Committable suggestion was skipped due to low confidence.
| return grpc.DialContext( //nolint:staticcheck | ||
| ctx, | ||
| endpoint, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithBlock(), | ||
| grpc.WithBlock(), //nolint:staticcheck |
There was a problem hiding this comment.
Tip
Codebase Verification
Documentation for //nolint:staticcheck annotations is insufficient.
The search results indicate multiple instances of //nolint:staticcheck across various files, but there is no accompanying documentation or comments explaining the usage or justification for these suppressions.
- File:
protocol/daemons/types/grpc_client.go- Lines: 51-55
Please ensure that the usage of //nolint:staticcheck is justified and documented to maintain code quality and clarity.
Analysis chain
Review of static analysis suppression annotations in TCP connection setup.
The //nolint:staticcheck annotations are used here as well. Similar to the previous function, ensure that these suppressions are justified and documented.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that these suppressions are documented elsewhere in the repository.
# Test: Search for documentation on nolint usage. Expect: Documentation or comments explaining the usage.
rg "//nolint:staticcheck" -C 5
Length of output: 59316
| return grpc.DialContext( //nolint:staticcheck | ||
| ctx, | ||
| socketAddress, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| // https://github.com/grpc/grpc-go/blob/master/dialoptions.go#L264 | ||
| grpc.WithBlock(), | ||
| grpc.WithBlock(), //nolint:staticcheck |
There was a problem hiding this comment.
Tip
Codebase Verification
Lack of justification for //nolint:staticcheck suppressions
The //nolint:staticcheck annotations in the code are not accompanied by any documentation or comments explaining their necessity. This lack of justification could lead to potential issues being overlooked.
- File:
protocol/daemons/types/grpc_client.go - Lines: 32-37
Please provide documentation or comments explaining why these static analysis warnings are being suppressed.
Analysis chain
Review of static analysis suppression annotations.
The use of //nolint:staticcheck suppresses warnings from static analysis tools. Ensure that these suppressions are justified and documented, explaining why they are necessary.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that these suppressions are documented elsewhere in the repository.
# Test: Search for documentation on nolint usage. Expect: Documentation or comments explaining the usage.
rg "//nolint:staticcheck" -C 5
Length of output: 59316
protocol/app/app.go
Outdated
| @@ -1594,15 +1593,14 @@ func (app *App) initOracle(pricesTxDecoder process.UpdateMarketPriceTxDecoder) { | |||
| compression.NewDefaultVoteExtensionCodec(), | |||
| compression.NewZLibCompressor(), | |||
| ), | |||
| // We are not using the slinky PreBlocker, so there is no need to pass in PreBlocker here for | |||
| // We are not using the slinky PreBlocker/PriceApplier, so there is no need to pass in PreBlocker here for | |||
There was a problem hiding this comment.
Are the PreBlocker comments still relevant after this upgrade w.r.t slinky? Should this comment just call out the lack of use of the slinky PriceApplier?
There was a problem hiding this comment.
The PreBlocker comment is no longer relevant (VE handler no longer depends on PreBlocker) so I'm aligned w/ removing references to PreBlocker in this commnet
There was a problem hiding this comment.
Probably not since we've moved away from using the preblocker here and have a dedicated interface now instead. Will remove.
| func (k Keeper) GetNumCurrencyPairs(ctx sdk.Context) (uint64, error) { | ||
| marketPriceStore := k.getMarketPriceStore(ctx) | ||
|
|
||
| var numMarketPrices uint64 | ||
|
|
||
| iterator := marketPriceStore.Iterator(nil, nil) | ||
| defer iterator.Close() | ||
|
|
||
| for ; iterator.Valid(); iterator.Next() { | ||
| marketPrice := types.MarketPrice{} | ||
| k.cdc.MustUnmarshal(iterator.Value(), &marketPrice) | ||
| numMarketPrices++ | ||
| } | ||
|
|
||
| return numMarketPrices, nil | ||
| } |
There was a problem hiding this comment.
This is a 1:1 copy of GetPrevBlockCPCounter, looking at the OracleKeeper interface, it doesn't seem like we need the GetPrevBlockCPCounter method anymore?
protocol/app/app.go
Outdated
| @@ -1594,15 +1593,14 @@ func (app *App) initOracle(pricesTxDecoder process.UpdateMarketPriceTxDecoder) { | |||
| compression.NewDefaultVoteExtensionCodec(), | |||
| compression.NewZLibCompressor(), | |||
| ), | |||
| // We are not using the slinky PreBlocker, so there is no need to pass in PreBlocker here for | |||
| // We are not using the slinky PreBlocker/PriceApplier, so there is no need to pass in PreBlocker here for | |||
There was a problem hiding this comment.
The PreBlocker comment is no longer relevant (VE handler no longer depends on PreBlocker) so I'm aligned w/ removing references to PreBlocker in this commnet
| return 0, nil | ||
| } | ||
|
|
||
| func (k Keeper) GetAllCurrencyPairs(ctx sdk.Context) []slinkytypes.CurrencyPair { |
There was a problem hiding this comment.
this fn should not be used in either the Delta / Default currency-pair strategies (which are what we'd be using in this case), so I'm happy to nop this / mark it explicitly as unimplemented
There was a problem hiding this comment.
Yeah I thought about that, but it seemed safer to implement it since we could so that nobody expected this function to work and it silently failed?
There was a problem hiding this comment.
Is there any issue with GetNumCurrencyPairs returning a number < the length of the slice returned by this method?
There's some extra validation logic in this function that could lead to this happening.
There was a problem hiding this comment.
I think I'm going to no-op this as Nikhil suggested--it's actually not being used at all right now.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/app/vote_extensions/expected_keepers.go (1)
[!TIP]
Codebase VerificationLine range hint
11-16: Interface Review: Ensure proper integration and usage ofPricesKeepermethods post-removal ofGetPrevBlockCPCounter.The
GetPrevBlockCPCountermethod has been successfully removed from thePricesKeeperinterface, and there are no remaining references to this method in the active codebase. However, the mock implementation inprotocol/mocks/PricesKeeper.gostill contains this method and should be updated to reflect the current interface.
protocol/mocks/PricesKeeper.go: Remove theGetPrevBlockCPCountermethod from the mock implementation.Analysis chain
Line range hint
11-16: Interface Review: Ensure proper integration and usage ofPricesKeepermethods post-removal ofGetPrevBlockCPCounter.The interface
PricesKeepercurrently defines methods related to market prices and currency pairs. Given the removal of theGetPrevBlockCPCountermethod, it's crucial to ensure that all components that previously depended on this method have been appropriately updated or refactored to work without it.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no remaining parts of the codebase are trying to use the removed `GetPrevBlockCPCounter` method. # Test: Search for the method usage. Expect: No occurrences found. rg --type go 'GetPrevBlockCPCounter'Length of output: 348
Script:
#!/bin/bash # Description: Verify if the mock implementation of PricesKeeper with the GetPrevBlockCPCounter method is still in use. # Test: Search for references to the PricesKeeper mock implementation. Expect: No relevant occurrences found. rg --type go 'PricesKeeper' --files-with-matches | xargs rg 'GetPrevBlockCPCounter'Length of output: 393
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/app/app.go (3 hunks)
- protocol/app/vote_extensions/expected_keepers.go (1 hunks)
- protocol/x/prices/keeper/slinky_adapter.go (2 hunks)
- protocol/x/prices/keeper/slinky_adapter_test.go (1 hunks)
- protocol/x/prices/types/types.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/prices/types/types.go
Files skipped from review as they are similar to previous changes (3)
- protocol/app/app.go
- protocol/x/prices/keeper/slinky_adapter.go
- protocol/x/prices/keeper/slinky_adapter_test.go
| @@ -113,3 +113,24 @@ func TestGetPrevBlockCPCounter(t *testing.T) { | |||
| require.NoError(t, err) | |||
| require.Equal(t, uint64(marketNumber), cpCounter) | |||
| } | |||
|
|
|||
| func TestGetAllCurrencyPairs(t *testing.T) { | |||
There was a problem hiding this comment.
nit: Perhaps also a test that this returns less curreny paris if one has an invalid format?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/prices/keeper/slinky_adapter.go (2 hunks)
- protocol/x/prices/keeper/slinky_adapter_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/prices/keeper/slinky_adapter.go
- protocol/x/prices/keeper/slinky_adapter_test.go
|
@Mergifyio backport release/protocol/v5.1.x |
✅ Backports have been created
|
(cherry picked from commit 6abbc72) # Conflicts: # protocol/mocks/OracleClient.go # protocol/x/prices/keeper/slinky_adapter_test.go
Co-authored-by: Eric Warehime <[email protected]> Co-authored-by: Vincent Chau <[email protected]>
Changelist
Updates the slinky version, compat methods.
Test Plan
Changes are non state breaking. Can be rolled out in dev/testnet on an isolated basis.
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
NoopPriceApplierfor handling price application and retrieval in vote extensions.Refactor
Bug Fixes
//nolint:staticcheckannotations to suppress static check warnings in gRPC connection functions.Tests
Dependencies