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.

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

feat: Slinky full integration PR #1141

Merged
merged 19 commits into from
Mar 20, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
4 changes: 2 additions & 2 deletions protocol/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ FROM golang@sha256:${GOLANG_1_22_ALPINE_DIGEST}
RUN apk add --no-cache bash

COPY --from=builder /dydxprotocol/build/dydxprotocold /bin/dydxprotocold
COPY --from=builder /dydxprotocol/daemons/slinky/config/oracle.json /etc/oracle.json
COPY --from=builder /dydxprotocol/daemons/slinky/config/market.json /etc/market.json
COPY --from=builder /slinky/build/oracle /bin/slinky
COPY --from=builder /slinky/config/local/oracle.json /etc/oracle.json
COPY --from=builder /slinky/config/local/market.json /etc/market.json

ENV HOME /dydxprotocol
WORKDIR $HOME
Expand Down
225 changes: 175 additions & 50 deletions protocol/app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,10 @@ import (
"github.com/cosmos/ibc-go/modules/capability"
capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper"
capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types"
"github.com/dydxprotocol/v4-chain/protocol/daemons/configs"
"github.com/gorilla/mux"
"github.com/rakyll/statik/fs"
"github.com/spf13/cast"
"go.uber.org/zap"
"google.golang.org/grpc"

// App
Comment on lines 89 to 98
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [738-738]

Given the static analysis hint and the context of the PR, it's crucial to ensure secure communication for gRPC servers. The setup for the gRPC server lacks encryption, which could expose sensitive data or allow unauthorized access. Consider using SSL/TLS credentials for a secure connection.

- app.Server = daemonserver.NewServer(logger, grpc.NewServer(), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)
+ app.Server = daemonserver.NewServer(logger, grpc.NewServer(grpc.Creds(credentials.NewServerTLSFromFile("cert.pem", "cert.key"))), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)

Expand All @@ -103,6 +103,7 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/app/prepare/prices"
"github.com/dydxprotocol/v4-chain/protocol/app/process"

"github.com/dydxprotocol/v4-chain/protocol/app/vote_extensions"
"github.com/dydxprotocol/v4-chain/protocol/lib"
"github.com/dydxprotocol/v4-chain/protocol/lib/metrics"
timelib "github.com/dydxprotocol/v4-chain/protocol/lib/time"
Expand All @@ -113,6 +114,7 @@ import (

// Daemons
bridgeclient "github.com/dydxprotocol/v4-chain/protocol/daemons/bridge/client"
"github.com/dydxprotocol/v4-chain/protocol/daemons/configs"
daemonflags "github.com/dydxprotocol/v4-chain/protocol/daemons/flags"
liquidationclient "github.com/dydxprotocol/v4-chain/protocol/daemons/liquidation/client"
metricsclient "github.com/dydxprotocol/v4-chain/protocol/daemons/metrics/client"
Expand Down Expand Up @@ -205,9 +207,16 @@ import (
"github.com/dydxprotocol/v4-chain/protocol/indexer/msgsender"

// Slinky
slinkyproposals "github.com/skip-mev/slinky/abci/proposals"
"github.com/skip-mev/slinky/abci/strategies/aggregator"
compression "github.com/skip-mev/slinky/abci/strategies/codec"
"github.com/skip-mev/slinky/abci/strategies/currencypair"
"github.com/skip-mev/slinky/abci/ve"
oracleconfig "github.com/skip-mev/slinky/oracle/config"
"github.com/skip-mev/slinky/pkg/math/voteweighted"
oracleclient "github.com/skip-mev/slinky/service/clients/oracle"
servicemetrics "github.com/skip-mev/slinky/service/metrics"
promserver "github.com/skip-mev/slinky/service/servers/prometheus"

// Grpc Streaming
streaming "github.com/dydxprotocol/v4-chain/protocol/streaming/grpc"
Expand Down Expand Up @@ -327,11 +336,13 @@ type App struct {
PriceFeedClient *pricefeedclient.Client
LiquidationsClient *liquidationclient.Client
BridgeClient *bridgeclient.Client
SlinkyClient *slinkyclient.Client

DaemonHealthMonitor *daemonservertypes.HealthMonitor

// Slinky
SlinkyClient *slinkyclient.Client
oraclePrometheusServer *promserver.PrometheusServer
oracleMetrics servicemetrics.Metrics
}

// assertAppPreconditions assert invariants required for an application to start.
Expand Down Expand Up @@ -1356,51 +1367,10 @@ func New(
app.SetPrecommiter(app.Precommitter)
app.SetPrepareCheckStater(app.PrepareCheckStater)

// PrepareProposal setup.
priceUpdateGenerator := prices.NewDefaultPriceUpdateGenerator(app.PricesKeeper)
if appFlags.NonValidatingFullNode {
app.SetPrepareProposal(prepare.FullNodePrepareProposalHandler())
} else {
app.SetPrepareProposal(
prepare.PrepareProposalHandler(
txConfig,
app.BridgeKeeper,
app.ClobKeeper,
app.PerpetualsKeeper,
priceUpdateGenerator,
),
)
}

// ProcessProposal setup.
priceUpdateDecoder := process.NewDefaultUpdateMarketPriceTxDecoder(app.PricesKeeper, app.txConfig.TxDecoder())
if appFlags.NonValidatingFullNode {
// Note: If the command-line flag `--non-validating-full-node` is enabled, this node will use
// an implementation of `ProcessProposal` which always returns `abci.ResponseProcessProposal_ACCEPT`.
// Full-nodes do not participate in consensus, and therefore should not participate in voting / `ProcessProposal`.
app.SetProcessProposal(
process.FullNodeProcessProposalHandler(
txConfig,
app.BridgeKeeper,
app.ClobKeeper,
app.StakingKeeper,
app.PerpetualsKeeper,
priceUpdateDecoder,
),
)
} else {
app.SetProcessProposal(
process.ProcessProposalHandler(
txConfig,
app.BridgeKeeper,
app.ClobKeeper,
app.StakingKeeper,
app.PerpetualsKeeper,
app.PricesKeeper,
priceUpdateDecoder,
),
)
}
// ProposalHandler setup.
prepareProposalHandler, processProposalHandler := app.createProposalHandlers(appFlags, txConfig, appOpts)
app.SetPrepareProposal(prepareProposalHandler)
app.SetProcessProposal(processProposalHandler)

// Note that panics from out of gas errors won't get logged, since the `OutOfGasMiddleware` is added in front of this,
// so error will get handled by that middleware and subsequent middlewares won't get executed.
Expand Down Expand Up @@ -1450,8 +1420,7 @@ func New(
return app
}

func (app *App) initSlinkySidecarClient(appOpts servertypes.AppOptions) oracleclient.OracleClient {
// Slinky setup
func (app *App) initOracleMetrics(appOpts servertypes.AppOptions) servicemetrics.Metrics {
cfg, err := oracleconfig.ReadConfigFromAppOpts(appOpts)
if err != nil {
panic(err)
Expand All @@ -1460,18 +1429,171 @@ func (app *App) initSlinkySidecarClient(appOpts servertypes.AppOptions) oraclecl
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()
}
return oracleMetrics
}

func (app *App) initSlinkySidecarClient(appOpts servertypes.AppOptions) oracleclient.OracleClient {
// Create the oracle service.
if app.oracleMetrics == nil {
app.oracleMetrics = app.initOracleMetrics(appOpts)
}
cfg, err := oracleconfig.ReadConfigFromAppOpts(appOpts)
if err != nil {
panic(err)
}
slinkyClient, err := oracleclient.NewClientFromConfig(
cfg,
app.Logger().With("client", "oracle"),
oracleMetrics,
app.oracleMetrics,
)
if err != nil {
panic(err)
}
return slinkyClient
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to initOracleMetrics, the initSlinkySidecarClient function panics on error when reading configuration or creating a new client. This abrupt termination might not be the most user-friendly approach. It's recommended to handle errors more gracefully by logging them and returning them to the caller, allowing for a more controlled response to configuration issues or failures in client creation.


func (app *App) createProposalHandlers(
appFlags flags.Flags,
txConfig client.TxConfig,
appOpts servertypes.AppOptions
) (sdk.PrepareProposalHandler, sdk.ProcessProposalHandler) {

strategy := currencypair.NewDefaultCurrencyPairStrategy(app.PricesKeeper)
veCodec := compression.NewCompressionVoteExtensionCodec(
compression.NewDefaultVoteExtensionCodec(),
compression.NewZLibCompressor(),
)
extCommitCodec := compression.NewCompressionExtendedCommitCodec(
compression.NewDefaultExtendedCommitCodec(),
compression.NewZLibCompressor(),
)

var priceUpdateGenerator prices.PriceUpdateGenerator = prices.NewDefaultPriceUpdateGenerator(app.PricesKeeper)
var priceUpdateDecoder process.UpdateMarketPriceTxDecoder = process.NewDefaultUpdateMarketPriceTxDecoder(
app.PricesKeeper, app.txConfig.TxDecoder())

if appFlags.VEOracleEnabled {
priceUpdateGenerator = prices.NewSlinkyPriceUpdateGenerator(
aggregator.NewDefaultVoteAggregator(
app.Logger(),
voteweighted.MedianFromContext(
app.Logger(),
app.StakingKeeper,
voteweighted.DefaultPowerThreshold,
),
strategy,
),
extCommitCodec,
veCodec,
strategy,
)
priceUpdateDecoder = process.NewSlinkyMarketPriceDecoder(
priceUpdateDecoder,
priceUpdateGenerator,
)
}
dydxPrepareProposalHandler := prepare.PrepareProposalHandler(
txConfig,
app.BridgeKeeper,
app.ClobKeeper,
app.PerpetualsKeeper,
priceUpdateGenerator,
)
// if the node is a NonValidatingFullNode, we don't need to run any of the oracle code
if appFlags.NonValidatingFullNode {
dydxPrepareProposalHandler = prepare.FullNodePrepareProposalHandler()
} else {
if app.oracleMetrics == nil {
app.oracleMetrics = app.initOracleMetrics(appOpts)
}
app.initOracle(priceUpdateDecoder)
}
if app.oracleMetrics == nil {
app.oracleMetrics = servicemetrics.NewNopMetrics()
}

// ProcessProposal setup.
var dydxProcessProposalHandler sdk.ProcessProposalHandler
if appFlags.NonValidatingFullNode {
// Note: If the command-line flag `--non-validating-full-node` is enabled, this node will use
// an implementation of `ProcessProposal` which always returns `abci.ResponseProcessProposal_ACCEPT`.
// Full-nodes do not participate in consensus, and therefore should not participate in voting / `ProcessProposal`.
dydxProcessProposalHandler = process.FullNodeProcessProposalHandler(
txConfig,
app.BridgeKeeper,
app.ClobKeeper,
app.StakingKeeper,
app.PerpetualsKeeper,
priceUpdateDecoder,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

if NonValidatingFullNode and VEOracleEnabled, right now you are returning the VE process proposal handler instead of the full node process proposal handler. Shouldn't we return dydxPrepareProposalHandler, dydxProcessProposalHandler here?

} else {
dydxProcessProposalHandler = process.ProcessProposalHandler(
txConfig,
app.BridgeKeeper,
app.ClobKeeper,
app.StakingKeeper,
app.PerpetualsKeeper,
app.PricesKeeper,
priceUpdateDecoder,
)
}

if appFlags.VEOracleEnabled {
proposalHandler := slinkyproposals.NewProposalHandler(
app.Logger(),
dydxPrepareProposalHandler,
dydxProcessProposalHandler,
ve.NewDefaultValidateVoteExtensionsFn(app.ChainID(), app.StakingKeeper),
veCodec,
extCommitCodec,
strategy,
app.oracleMetrics,
slinkyproposals.RetainOracleDataInWrappedProposalHandler(),
)
return proposalHandler.PrepareProposalHandler(), proposalHandler.ProcessProposalHandler()
} else {
return dydxPrepareProposalHandler, dydxProcessProposalHandler
}
}

func (app *App) initOracle(pricesTxDecoder process.UpdateMarketPriceTxDecoder) {
// Vote Extension setup.
slinkyVoteExtensionsHandler := ve.NewVoteExtensionHandler(
app.Logger(),
vote_extensions.NewOraclePrices(app.PricesKeeper),
time.Second,
currencypair.NewDefaultCurrencyPairStrategy(app.PricesKeeper),
compression.NewCompressionVoteExtensionCodec(
compression.NewDefaultVoteExtensionCodec(),
compression.NewZLibCompressor(),
),
app.PreBlocker,
app.oracleMetrics,
)

dydxExtendVoteHandler := vote_extensions.ExtendVoteHandler{
SlinkyExtendVoteHandler: slinkyVoteExtensionsHandler.ExtendVoteHandler(),
PricesTxDecoder: pricesTxDecoder,
PricesKeeper: app.PricesKeeper,
}

app.SetExtendVoteHandler(dydxExtendVoteHandler.ExtendVoteHandler())
app.SetVerifyVoteExtensionHandler(slinkyVoteExtensionsHandler.VerifyVoteExtensionHandler())
}

// RegisterDaemonWithHealthMonitor registers a daemon service with the update monitor, which will commence monitoring
// the health of the daemon. If the daemon does not register, the method will panic.
func (app *App) RegisterDaemonWithHealthMonitor(
Expand Down Expand Up @@ -1746,6 +1868,9 @@ func (app *App) setAnteHandler(txConfig client.TxConfig) {
// Close invokes an ordered shutdown of routines.
func (app *App) Close() error {
app.BaseApp.Close()
if app.oraclePrometheusServer != nil {
app.oraclePrometheusServer.Close()
}
Comment on lines +1864 to +1866
Copy link
Contributor

Choose a reason for hiding this comment

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

Closing the oraclePrometheusServer without checking if an error occurred during the shutdown can lead to unnoticed issues. It's a good practice to log or handle errors even during shutdown processes.

- app.oraclePrometheusServer.Close()
+ if err := app.oraclePrometheusServer.Close(); err != nil {
+     app.Logger().Error("Failed to close oracle Prometheus server", "error", err)
+ }

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.

Suggested change
if app.oraclePrometheusServer != nil {
app.oraclePrometheusServer.Close()
}
if app.oraclePrometheusServer != nil {
if err := app.oraclePrometheusServer.Close(); err != nil {
app.Logger().Error("Failed to close oracle Prometheus server", "error", err)
}
}

return app.closeOnce()
}

Expand Down
1 change: 1 addition & 0 deletions protocol/app/app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ func TestAppIsFullyInitialized(t *testing.T) {
"LiquidationsClient",
"BridgeClient",
"SlinkyClient",
"oraclePrometheusServer",

// Any default constructed type can be considered initialized if the default is what is
// expected. getUninitializedStructFields relies on fields being the non-default and
Expand Down
16 changes: 16 additions & 0 deletions protocol/app/flags/flags.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type Flags struct {

// Grpc Streaming
GrpcStreamingEnabled bool
VEOracleEnabled bool // Slinky Vote Extensions
}

// List of CLI flags.
Expand All @@ -37,6 +38,9 @@ const (

// Grpc Streaming
GrpcStreamingEnabled = "grpc-streaming-enabled"

// Slinky VEs enabled
VEOracleEnabled = "slinky-vote-extension-oracle-enabled"
)

// Default values.
Expand All @@ -47,6 +51,7 @@ const (
DefaultDdErrorTrackingFormat = false

DefaultGrpcStreamingEnabled = false
DefaultVEOracleEnabled = false
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we want this to be enable to true for the next release?

)

// AddFlagsToCmd adds flags to app initialization.
Expand Down Expand Up @@ -80,6 +85,11 @@ func AddFlagsToCmd(cmd *cobra.Command) {
DefaultGrpcStreamingEnabled,
"Whether to enable grpc streaming for full nodes",
)
cmd.Flags().Bool(
VEOracleEnabled,
DefaultVEOracleEnabled,
"Whether to run on-chain oracle via slinky vote extensions",
)
}

// Validate checks that the flags are valid.
Expand Down Expand Up @@ -164,5 +174,11 @@ func GetFlagValuesFromOptions(
}
}

if option := appOpts.Get(VEOracleEnabled); option != nil {
if v, err := cast.ToBoolE(option); err == nil {
result.VEOracleEnabled = v
}
}

return result
}
2 changes: 1 addition & 1 deletion protocol/app/prepare/full_node_prepare_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@ func FullNodePrepareProposalHandler() sdk.PrepareProposalHandler {
recordErrorMetricsWithLabel(metrics.PrepareProposalTxs)

// Return an empty response if the node is running in full-node mode so that the proposal fails.
return &EmptyResponse, nil
return &abci.ResponsePrepareProposal{Txs: [][]byte{}}, nil
}
}
Loading
Loading