Move clob hydration to preblocker #1412
Conversation
WalkthroughThe recent updates primarily focus on enhancing initialization and state management within the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedPath-based Instructions (1)
Additional comments not posted (3)
Warning Following problems were encountered
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: 0
Out of diff range and nitpick comments (1)
protocol/app/app.go (1)
Line range hint
739-739: Secure the gRPC server by adding SSL/TLS credentials.- app.Server = daemonserver.NewServer(logger, grpc.NewServer(), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress) + creds, err := credentials.NewServerTLSFromFile("cert.pem", "cert.key") + if err != nil { + panic(err) + } + app.Server = daemonserver.NewServer(logger, grpc.NewServer(grpc.Creds(creds)), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)
|
Non-blocking question, is this test-able in either an E2E test or the upgrade test? |
protocol/x/clob/ante/clob.go
Outdated
| @@ -60,6 +60,14 @@ func (cd ClobDecorator) AnteHandle( | |||
| return next(ctx, tx, simulate) | |||
| } | |||
|
|
|||
| // Disable order placement and cancelation processing if the clob keeper is not hydrated. | |||
| if !cd.clobKeeper.IsHydrated() { | |||
There was a problem hiding this comment.
Nit: would it makes sense to name this field more generically? Hydration is one of the things that could be done here
something like IsInitialized? This pattern can be consistent across modules
protocol/x/clob/keeper/keeper.go
Outdated
| @@ -111,6 +112,7 @@ func NewKeeper( | |||
| indexerEventManager: indexerEventManager, | |||
| streamingManager: grpcStreamingManager, | |||
| memStoreInitialized: &atomic.Bool{}, | |||
| hydrated: &atomic.Bool{}, | |||
There was a problem hiding this comment.
Nit: readability, can we document this is initialize as false
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
protocol/x/clob/keeper/keeper.go (1)
48-48: Initialization ofinitializedandmemStoreInitializedusingatomic.Boolis appropriate for concurrency control. Consider adding documentation to these fields to clarify their purpose, as suggested in previous comments.protocol/app/app.go (1)
Line range hint
739-739: Secure the gRPC server by adding SSL/TLS credentials.The gRPC server is initialized without specifying SSL/TLS credentials, which could allow unencrypted connections. This is a security risk as it could allow an attacker to tamper with the gRPC messages. Consider using SSL/TLS to secure the connection.
- grpc.NewServer(), + grpc.NewServer(grpc.Creds(credentials.NewServerTLSFromFile("cert.pem", "cert.key"))),
jayy04
force-pushed
the
jy/hydration-preblocker
branch
from
0d38579
to
0c7b85c
Compare
| // maybeCreateOrderbook creates a new orderbook in the memclob. | ||
| func (k Keeper) maybeCreateOrderbook(ctx sdk.Context, clobPair types.ClobPair) { | ||
| // Create the corresponding orderbook in the memclob. | ||
| k.MemClob.MaybeCreateOrderbook(ctx, clobPair) | ||
| } |
There was a problem hiding this comment.
need maybe create orderbook here since orderbooks might have already been created by InitGenesis
protocol/x/clob/keeper/keeper.go
Outdated
| // Branch the context for hydration. | ||
| // This means that new order matches from hydration will get added to the operations | ||
| // queue but the corresponding state changes will be discarded. | ||
| // This is because we are currently in the deliver state so writing optimistic matches | ||
| // breaks consensus. | ||
| checkCtx, _ := ctx.CacheContext() | ||
| checkCtx = checkCtx.WithIsCheckTx(true) |
There was a problem hiding this comment.
someone give me a quick sanity check here
There was a problem hiding this comment.
Logic / comment makes sense, however seems hard to track. Maybe an additional comment that this is currently only run in Preblocker which is why this is needed?
protocol/x/clob/ante/clob.go
Outdated
| @@ -60,6 +60,14 @@ func (cd ClobDecorator) AnteHandle( | |||
| return next(ctx, tx, simulate) | |||
| } | |||
|
|
|||
| // Disable order placement and cancelation processing if the clob keeper is not hydrated. | |||
There was a problem hiding this comment.
nit: Replace hydrated with initialized.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
protocol/app/app.go (1)
Line range hint
740-740: Secure the gRPC server to prevent unauthorized access.- app.Server = daemonserver.NewServer(logger, grpc.NewServer(), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress) + creds, err := credentials.NewServerTLSFromFile("cert.pem", "cert.key") + if err != nil { + panic(err) + } + app.Server = daemonserver.NewServer(logger, grpc.NewServer(grpc.Creds(creds)), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)The gRPC server is currently initialized without any credentials, which could expose it to unauthorized access and tampering. It's recommended to secure the server using SSL/TLS credentials.
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
protocol/app/app.go (2)
Line range hint
740-740: Secure the gRPC server by adding SSL/TLS credentials.- app.Server = daemonserver.NewServer(logger, grpc.NewServer(), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress) + creds, err := credentials.NewServerTLSFromFile("cert.pem", "cert.key") + if err != nil { + panic(err) + } + app.Server = daemonserver.NewServer(logger, grpc.NewServer(grpc.Creds(creds)), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)Ensure that the server uses SSL/TLS credentials to prevent unauthorized access and data tampering.
1623-1629: Ensure clarity in the comment about gas meter handling inPreBlocker.The comment about setting the gas meter to a free infinite gas meter could be expanded to explain why this is necessary and what specific non-deterministic operations require this setting. This would help future maintainers understand the reasoning behind this design choice.
|
@Mergifyio backport release/protocol/v5.x |
✅ Backports have been created
|
* Move clob hydration to preblocker (#1412) * Move clob hydration to preblocker * updates * comments * nit * reset gas meter; comments * remove defer * fix test * fix test * e2e test for hydration in preblocker (#1437) * e2e test for hydration in preblocker * fix lint (cherry picked from commit 20113d2) * move e2e test app instantiation to Build (#1422) --------- Co-authored-by: jayy04 <103467857+jayy04@users.noreply.github.com>
Changelist
[Describe or list the changes made in this PR]
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.