Skip to content
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.

Sign up for GitHub

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

chore: Update slinky version #1768

Merged
merged 7 commits into from
Jun 26, 2024
Merged

chore: Update slinky version #1768

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3a72cb6
Update slinky version
Eric-Warehime Jun 25, 2024
8e9ace0
Bump to slinky commit
Eric-Warehime Jun 25, 2024
ae42e73
Update oracle client mock
Eric-Warehime Jun 25, 2024
ce7bfed
Fix tests, lint
Eric-Warehime Jun 25, 2024
2fecfec
Separate replace block
Eric-Warehime Jun 25, 2024
14fe18d
PR comments
Eric-Warehime Jun 25, 2024
eab0745
Remove unnecessary impl
Eric-Warehime Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 3 additions & 18 deletions protocol/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ import (
"github.com/gorilla/mux"
"github.com/rakyll/statik/fs"
"github.com/spf13/cast"
"go.uber.org/zap"
"google.golang.org/grpc"

// App
Expand Down Expand Up @@ -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
Copy link
Contributor

@vincentwschau vincentwschau Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not since we've moved away from using the preblocker here and have a dedicated interface now instead. Will remove.

// 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{},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

app.oracleMetrics,
)

Expand All @@ -1625,19 +1623,6 @@ func (app *App) initOracleMetrics(appOpts servertypes.AppOptions) {
if err != nil {
panic(err)
}
// run prometheus metrics
if cfg.MetricsEnabled {
promLogger, err := zap.NewProduction()
if err != nil {
panic(err)
}
app.oraclePrometheusServer, err = promserver.NewPrometheusServer(cfg.PrometheusServerAddress, promLogger)
if err != nil {
panic(err)
}
// start the prometheus server
go app.oraclePrometheusServer.Start()
}
app.oracleMetrics = oracleMetrics
}

Expand Down
12 changes: 12 additions & 0 deletions protocol/app/vote_extensions/extend_vote_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package vote_extensions

import (
"fmt"
slinkytypes "github.com/skip-mev/slinky/pkg/types"
"math/big"

cometabci "github.com/cometbft/cometbft/abci/types"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -19,6 +21,16 @@ type ExtendVoteHandler struct {
PricesKeeper PricesKeeper
}

type NoopPriceApplier struct{}

func (n NoopPriceApplier) ApplyPricesFromVoteExtensions(
_ sdk.Context, _ *cometabci.RequestFinalizeBlock) (map[slinkytypes.CurrencyPair]*big.Int, error) {
return nil, nil
}
func (n NoopPriceApplier) GetPricesForValidator(_ sdk.ConsAddress) map[slinkytypes.CurrencyPair]*big.Int {
return nil
}

// ExtendVoteHandler returns a sdk.ExtendVoteHandler, responsible for the following:
// 1. Decoding the x/prices MsgUpdateMarketPrices in the current block - fail on errors
// 2. Validating the proposed MsgUpdateMarketPrices in accordance with the ProcessProposal check
Expand Down
9 changes: 4 additions & 5 deletions protocol/cmd/dydxprotocold/cmd/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,10 @@ func initAppConfig() (string, *DydxAppConfig) {
appConfig := DydxAppConfig{
Config: *srvCfg,
Oracle: oracleconfig.AppConfig{
Enabled: true,
OracleAddress: "localhost:8080",
ClientTimeout: time.Second * 2,
MetricsEnabled: false,
PrometheusServerAddress: "",
Enabled: true,
OracleAddress: "localhost:8080",
ClientTimeout: time.Second * 2,
MetricsEnabled: false,
Comment on lines +58 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
}

Expand Down
19 changes: 4 additions & 15 deletions protocol/daemons/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,10 @@ func GetDefaultDaemonFlags() DaemonFlags {
},
Slinky: SlinkyFlags{
AppConfig: oracleconfig.AppConfig{
Enabled: true,
OracleAddress: "localhost:8080",
ClientTimeout: time.Second * 2,
MetricsEnabled: true,
PrometheusServerAddress: "0.0.0.0:8001",
Enabled: true,
OracleAddress: "localhost:8080",
ClientTimeout: time.Second * 2,
MetricsEnabled: true,
},
},
}
Expand Down Expand Up @@ -218,11 +217,6 @@ func AddDaemonFlagsToCmd(
df.Slinky.AppConfig.MetricsEnabled,
"Enable the oracle metrics reporting for Slinky.",
)
cmd.Flags().String(
FlagOraclePrometheusServerAddress,
df.Slinky.AppConfig.PrometheusServerAddress,
"The address of the exposed prometheus address for Slinky metrics.",
)
}

// GetDaemonFlagValuesFromOptions gets all daemon flag values from the `AppOptions` struct.
Expand Down Expand Up @@ -316,11 +310,6 @@ func GetDaemonFlagValuesFromOptions(
result.Slinky.AppConfig.MetricsEnabled = v
}
}
if option := appOpts.Get(FlagOraclePrometheusServerAddress); option != nil {
if v, err := cast.ToStringE(option); err == nil {
result.Slinky.AppConfig.PrometheusServerAddress = v
}
}

return result
}
8 changes: 4 additions & 4 deletions protocol/daemons/types/grpc_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ func (g *GrpcClientImpl) NewGrpcConnection(
ctx context.Context,
socketAddress string,
) (*grpc.ClientConn, error) {
return grpc.DialContext(
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
Comment on lines +32 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
// Create a custom `net.Dialer` in order to specify `unix` as the desired network.
var dialer net.Dialer
Expand All @@ -48,11 +48,11 @@ func (g *GrpcClientImpl) NewTcpConnection(
ctx context.Context,
endpoint string,
) (*grpc.ClientConn, error) {
return grpc.DialContext(
return grpc.DialContext( //nolint:staticcheck
ctx,
endpoint,
grpc.WithTransportCredentials(insecure.NewCredentials()),
grpc.WithBlock(),
grpc.WithBlock(), //nolint:staticcheck
Comment on lines +51 to +55
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)
}

Expand Down
Loading
Loading