feat(prophet): inital version #1141
Conversation
📝 WalkthroughWalkthroughThe pull request introduces significant updates across multiple files, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (14)
prophet/dedup.go (2)
13-14: Correct the function documentation for clarity.The comments have redundant words "and emits returns". It should be corrected for clarity.
Suggested change:
-// dedup takes a channel of requests, deduplicates them, and emits -// returns a new channel of unique requests. +// dedup takes a channel of requests, deduplicates them, and returns a new channel of unique requests.
31-31: Consider adding a buffer to the output channel.To improve performance and prevent potential blocking under high throughput, consider adding a buffer to the
outchannel.Suggested change:
-out := make(chan T) +out := make(chan T, dedupChannelBufferSize)Define
dedupChannelBufferSizewith an appropriate buffer size based on expected load.prophet/handlers.go (1)
23-26: Use descriptive variable names for better readability.The variable
scan be renamed tohandlerto improve code clarity.Suggested change:
In both functions, replace
swithhandler.- s := getHandler(f.Handler) + handler := getHandler(f.Handler) - if s == nil { + if handler == nil { - return FutureResult{}, fmt.Errorf("no future handler registered for %s", f.Handler) + return FutureResult{}, fmt.Errorf("no future handler registered for %s", f.Handler) } ... - s := getHandler(f.Handler) + handler := getHandler(f.Handler) - if s == nil { + if handler == nil { - return fmt.Errorf("no future handler registered for %s", f.Handler) + return fmt.Errorf("no future handler registered for %s", f.Handler) }Also applies to: 47-50
warden/x/async/keeper/abci.go (1)
70-81: Consider adding validation toVerifyVoteExtensionHandlerCurrently,
VerifyVoteExtensionHandleraccepts any vote extension that unmarshals successfully. It might be prudent to add additional validation to ensure the vote extension contents meet expected criteria, enhancing security and correctness.prophet/registry.go (1)
15-23: Consider using error return instead of panicWhile panics can be appropriate for truly unrecoverable situations, registration conflicts might be better handled by returning an error. This would allow callers to handle duplicate registrations gracefully.
-func Register(name string, future FutureHandler) { +func Register(name string, future FutureHandler) error { + if future == nil { + return fmt.Errorf("future handler cannot be nil") + } registry.rw.Lock() defer registry.rw.Unlock() if _, found := registry.futures[name]; found { - panic("future already registered") + return fmt.Errorf("future %q already registered", name) } registry.futures[name] = future + return nil }prophet/handlers/echo/echo.go (2)
18-20: Consider utilizing the context parameterThe Execute method ignores the context parameter. Consider adding timeout handling or cancellation support.
func (s Future) Execute(ctx context.Context, input []byte) ([]byte, error) { + if ctx.Err() != nil { + return nil, fmt.Errorf("context error: %w", ctx.Err()) + } return input, nil }
22-27: Add test coverage for the Echo handlerThe Echo handler needs unit tests to verify its behavior.
Would you like me to help create a test file with comprehensive test cases for the Echo handler?
proto/warden/async/v1beta1/ve.proto (1)
19-24: Enhance message documentationWhile the purpose is documented, consider adding more detailed field-level documentation for
resultsandvotes.// A vote extension coming from a validator. It contains results and votes for // some futures. message AsyncVoteExtension { + // results contains the outputs of completed futures repeated VEResultItem results = 1; + // votes contains the validator's votes on future results repeated VEVoteItem votes = 2; }prophet/future.go (3)
13-14: Consider documenting the getIDer interfaceThe comment references an undocumented
getIDerinterface. This interface should be documented for better code clarity.
21-25: Consider adding custom JSON marshaling for large byte arraysThe FutureResult embeds Future and adds Output as []byte. For large outputs, custom JSON marshaling might be beneficial to control the string representation.
35-43: Consider using typed errors for Vote.ErrThe Vote struct uses a generic error type. Consider defining specific error types for common verification failures to make error handling more precise.
Example:
type VerificationError struct { Code int Message string } func (e *VerificationError) Error() string { return e.Message }warden/x/async/keeper/keeper.go (1)
13-13: Consider documenting the Prophet integrationThe new Prophet field lacks documentation explaining its purpose and usage within the Keeper struct. According to the Uber Go Style Guide, exported types should be documented.
Add a comment explaining the Prophet field's purpose:
type Keeper struct { // ... other fields ... + // p is the Prophet instance responsible for managing asynchronous task execution p *prophet.P }Also applies to: 29-30
warden/x/async/keeper/abci_test.go (2)
9-73: Add test cases for negative maxSizeBytesThe TestTrimExcessBytes function doesn't test behavior with negative maxSizeBytes values.
Add a test case for negative maxSizeBytes:
{ name: "nil list", args: args{ txs: nil, maxSizeBytes: 8, }, want: nil, }, + { + name: "negative maxSizeBytes", + args: args{ + txs: [][]byte{[]byte("tx1"), []byte("tx2")}, + maxSizeBytes: -1, + }, + want: nil, + },
75-137: Add test for nil transaction in injectTxThe TestInjectTx function should verify behavior when injecting a nil transaction.
Add a test case for nil transaction:
{ name: "position in the middle", args: args{ newTx: []byte("newTx"), position: 1, appTxs: [][]byte{[]byte("appTx1"), []byte("appTx2")}, }, want: [][]byte{[]byte("appTx1"), []byte("newTx"), []byte("appTx2")}, }, + { + name: "nil transaction", + args: args{ + newTx: nil, + position: 1, + appTxs: [][]byte{[]byte("appTx1"), []byte("appTx2")}, + }, + want: [][]byte{[]byte("appTx1"), []byte("appTx2")}, + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/warden/async/v1beta1/ve.pulsar.gois excluded by!api/**api/warden/vemanager/vemanager.pulsar.gois excluded by!api/**warden/app/vemanager/vemanager.pb.gois excluded by!**/*.pb.gowarden/x/async/types/v1beta1/ve.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
CHANGELOG.md(1 hunks)go.mod(1 hunks)prophet/dedup.go(1 hunks)prophet/doc.go(1 hunks)prophet/exec.go(1 hunks)prophet/future.go(1 hunks)prophet/handlers.go(1 hunks)prophet/handlers/echo/echo.go(1 hunks)prophet/prophet.go(1 hunks)prophet/registry.go(1 hunks)proto/warden/async/v1beta1/ve.proto(1 hunks)proto/warden/vemanager/vemanager.proto(1 hunks)warden/app/app.go(6 hunks)warden/app/oracle.go(4 hunks)warden/app/vemanager/vemanager.go(1 hunks)warden/testutil/keeper/async.go(1 hunks)warden/x/async/keeper/abci.go(1 hunks)warden/x/async/keeper/abci_test.go(1 hunks)warden/x/async/keeper/futures.go(5 hunks)warden/x/async/keeper/keeper.go(5 hunks)warden/x/async/module/module.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- prophet/doc.go
🧰 Additional context used
📓 Path-based instructions (17)
warden/app/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/module/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/futures.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/registry.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/handlers/echo/echo.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/testutil/keeper/async.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
prophet/future.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/exec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/oracle.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/dedup.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
prophet/prophet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/vemanager/vemanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf (1.47.2)
proto/warden/vemanager/vemanager.proto
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
proto/warden/async/v1beta1/ve.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
🪛 golangci-lint (1.62.2)
prophet/prophet.go
82-82: Error return value of p.resultsWriter.Remove is not checked
(errcheck)
🔇 Additional comments (15)
prophet/handlers.go (1)
10-19: LGTM!
The FutureHandler interface is defined correctly and follows the required method signatures.
warden/x/async/keeper/abci.go (4)
19-36: Function EndBlocker implementation looks good
The EndBlocker function correctly processes pending futures and schedules them with the Prophet. Error handling and loop logic are appropriate.
40-65: ExtendVoteHandler implementation is correct
The ExtendVoteHandler function properly collects results from the Prophet, marshals them, and constructs the appropriate response. Error handling is adequate.
219-233: Function injectTx logic is correct
The injectTx function correctly handles the insertion of a new transaction at the specified position, with appropriate edge case handling for positions at the start or end of the transaction list.
235-248: Function trimExcessBytes implementation is efficient
The trimExcessBytes function effectively trims the transaction list to comply with MaxTxBytes, ensuring that the total size of included transactions does not exceed the limit.
warden/app/app.go (2)
248-252: Prophet initialization looks correct
The integration of the prophet instance is appropriately handled with error checking and panic on failure, ensuring the application does not proceed without a successfully initialized prophet.
406-409: Properly handling prophet.Run execution
The application correctly invokes app.prophet.Run() and handles any errors by panicking, which prevents the application from running in an inconsistent state if the prophet fails to start.
proto/warden/async/v1beta1/ve.proto (2)
11-17: Address the FIXME comment regarding data duplication
The comment indicates potential data duplication with slinky. This should be investigated and resolved to prevent unnecessary block space usage.
Consider:
- Refactoring to share the vote info between components
- Documenting why duplication might be necessary if it can't be avoided
- Adding a reference to a tracking issue for this optimization
5-7: Verify gogoproto import path
The static analysis indicates the gogoproto import might be incorrect. Verify the import path and ensure it's properly vendored.
✅ Verification successful
Let me verify the import path by checking the actual content and location of the proto file.
Let me check one more thing to verify the import usage pattern in the proto files.
Import path for gogoproto is correct
The import path gogoproto/gogo.proto is correctly used across the entire codebase. This is evidenced by:
- The presence of the file at
wardenjs/proto/gogoproto/gogo.proto - Consistent usage of the same import path in over 100 proto files across the codebase
- Proper dependency declaration in go.mod with
github.com/cosmos/gogoproto v1.7.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for gogoproto in vendor or go.mod
fd -t f "gogo.proto"
cat go.mod | grep -A 5 "gogo"
Length of output: 1070
Script:
#!/bin/bash
# Check the content of the gogo.proto file and its import usage
cat wardenjs/proto/gogoproto/gogo.proto
rg -l "import.*gogoproto/gogo.proto" --type proto
Length of output: 4993
Script:
#!/bin/bash
# Check for proto files that import gogoproto and their import paths
rg "^import.*gogoproto" --type-add 'proto:*.proto' --type proto -A 1
Length of output: 18776
🧰 Tools
🪛 buf (1.47.2)
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
warden/testutil/keeper/async.go (1)
41-41: Document the purpose of the nil parameter
The addition of a nil parameter to NewKeeper should be documented to explain its purpose and any implications for test scenarios.
Let's check the NewKeeper implementation:
✅ Verification successful
Let me check the test file to understand how the nil parameter is used.
Let me check the prophet parameter usage in the async keeper.
The nil parameter is for the prophet.P instance used in async consensus
The nil parameter in the test setup is intentional and safe. The prophet parameter is used in the production keeper for consensus-related operations (vote extensions and future handling), but it's not required for basic testing scenarios. The test utility appropriately passes nil since consensus operations are not being tested.
Key observations:
- The prophet parameter is used in production for:
- Adding futures via
p.AddFuture() - Processing vote extensions in ABCI handlers
- Adding futures via
- In test scenarios, passing nil is safe as these consensus features are not exercised
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewKeeper implementation
ast-grep --pattern 'func NewKeeper($$$) $$$'
Length of output: 12509
Script:
#!/bin/bash
# Search for the test file content
cat warden/testutil/keeper/async.go
Length of output: 1579
Script:
#!/bin/bash
# Search for prophet usage in async keeper
rg -A 5 -B 5 "prophet" warden/x/async/keeper/
Length of output: 4254
warden/x/async/module/module.go (3)
24-24: LGTM: Import statement added correctly.
The import statement for the prophet package is properly added.
194-195: LGTM: Optional Prophet field added correctly.
The Prophet field is properly added with the optional:"true" tag, following dependency injection best practices.
217-217: LGTM: Prophet parameter passed correctly to NewKeeper.
The Prophet instance is properly passed to the NewKeeper constructor.
go.mod (1)
62-62: Verify compatibility with golang-lru v2.x
The upgrade from golang-lru v1.0.2 to v2.0.7 is a major version change that may introduce breaking changes.
Run the following script to check for potential breaking changes:
✅ Verification successful
The golang-lru v2 upgrade is compatible with the codebase
The code is already using the v2 API correctly:
- All imports are using the v2 package (no direct imports of v1 found)
- The
lru.Newcalls are using proper generic type parameters (lru.New[uint64, T]), which is the v2 syntax - The
Cachetype is also used with generic parameters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of golang-lru v1.x APIs that might be incompatible with v2.x
# Search for New/NewWithEvict function calls that might need updates
rg -A 2 'lru\.New|lru\.NewWithEvict'
# Search for any direct imports of the old package
rg '"github.com/hashicorp/golang-lru"'
Length of output: 357
Script:
#!/bin/bash
# Let's check the actual import statements in the files that use lru
rg -p "import.*hashicorp/golang-lru" -A 5 prophet/prophet.go prophet/dedup.go
# Also check for any other potential uses of lru package
rg -p "lru\." -B 2 -A 2
Length of output: 666
CHANGELOG.md (1)
57-57: LGTM: Prophet module changelog entry is well-documented.
The changelog entry for the Prophet module follows the Keep a Changelog format and clearly describes its initial features.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
proto/warden/async/v1beta1/ve.proto (1)
19-24: Enhance documentation for AsyncVoteExtensionWhile the basic purpose is documented, consider adding:
- Field-level documentation for
resultsandvotes- Example usage or scenarios
prophet/dedup.go (2)
7-7: Consider making dedupLRUSize configurableThe LRU cache size is hardcoded to 10000 entries. Consider:
- Making it configurable via options pattern
- Documenting the rationale for the chosen size
51-67: Consider adding error handling for closed channelsThe
Read()method returns the output channel directly. Consider handling scenarios where the underlying dedup channel is closed unexpectedly.Example approach:
func (d dedupFutureReader) Read() <-chan Future { + // Create a buffered channel to handle closure + ch := make(chan Future, 100) + go func() { + defer close(ch) + for f := range d.d.out { + ch <- f + } + }() + return ch }prophet/prophet.go (3)
10-15: Consider making queueBufferSize configurable.The hard-coded buffer size of 100 might not be suitable for all deployment scenarios. Consider making this configurable through the constructor or environment variables.
-var queueBufferSize = 100 +// DefaultQueueBufferSize is the default size for incoming queues +const DefaultQueueBufferSize = 100 + +// Config holds Prophet configuration +type Config struct { + QueueBufferSize int +}
117-117: Consider making defaultSetSize configurable.Similar to queueBufferSize, the hard-coded set size of 10000 should be configurable to accommodate different use cases.
133-137: Consider error handling in Write and Remove methods.Both methods silently ignore potential errors from the LRU cache operations. While these operations are unlikely to fail, it would be more robust to handle potential errors.
func (s *s[T]) Write(value T) error { id := value.getID() - s.l.Add(id, value) + evicted := s.l.Add(id, value) + if evicted { + slog.Debug("value evicted from set", "id", id) + } return nil } func (s *s[T]) Remove(values ...T) error { + var errs []error for _, v := range values { - s.l.Remove(v.getID()) + if _, existed := s.l.Remove(v.getID()); !existed { + errs = append(errs, fmt.Errorf("value not found in set: %v", v.getID())) + } } + if len(errs) > 0 { + return fmt.Errorf("failed to remove some values: %v", errs) + } return nil }Also applies to: 143-148
warden/x/async/keeper/abci.go (1)
170-170: Address the TODO comment about vote extension signature verification.The comment indicates missing signature verification for vote extensions. This security check should be implemented.
Would you like me to help implement the vote extension signature verification or create a GitHub issue to track this task?
warden/app/oracle.go (1)
256-272: Consider early return on first handler errorThe current implementation continues to execute the second handler even if the first one fails. Consider returning early on the first error to prevent potentially unnecessary operations.
Apply this diff to add early return:
func combinePreBlocker(a, b sdk.PreBlocker) sdk.PreBlocker { return func(ctx sdk.Context, req *cometabci.RequestFinalizeBlock) (*sdk.ResponsePreBlock, error) { respA, err := a(ctx, req) if err != nil { - return respA, err + return nil, err } respB, err := b(ctx, req) if err != nil { - return respB, err + return nil, err } return &sdk.ResponsePreBlock{ ConsensusParamsChanged: respA.ConsensusParamsChanged || respB.ConsensusParamsChanged, }, nil } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/warden/async/v1beta1/ve.pulsar.gois excluded by!api/**api/warden/vemanager/vemanager.pulsar.gois excluded by!api/**warden/app/vemanager/vemanager.pb.gois excluded by!**/*.pb.gowarden/x/async/types/v1beta1/ve.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
CHANGELOG.md(1 hunks)go.mod(1 hunks)prophet/dedup.go(1 hunks)prophet/doc.go(1 hunks)prophet/exec.go(1 hunks)prophet/future.go(1 hunks)prophet/handlers.go(1 hunks)prophet/handlers/echo/echo.go(1 hunks)prophet/prophet.go(1 hunks)prophet/registry.go(1 hunks)proto/warden/async/v1beta1/ve.proto(1 hunks)proto/warden/vemanager/vemanager.proto(1 hunks)warden/app/app.go(6 hunks)warden/app/oracle.go(4 hunks)warden/app/vemanager/vemanager.go(1 hunks)warden/testutil/keeper/async.go(1 hunks)warden/x/async/keeper/abci.go(1 hunks)warden/x/async/keeper/abci_test.go(1 hunks)warden/x/async/keeper/futures.go(5 hunks)warden/x/async/keeper/keeper.go(5 hunks)warden/x/async/module/module.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- prophet/doc.go
- warden/testutil/keeper/async.go
- prophet/registry.go
- warden/x/async/keeper/futures.go
- go.mod
- prophet/handlers/echo/echo.go
- warden/x/async/keeper/abci_test.go
- prophet/exec.go
- prophet/handlers.go
- prophet/future.go
- warden/x/async/keeper/keeper.go
- warden/app/vemanager/vemanager.go
🧰 Additional context used
📓 Path-based instructions (7)
warden/x/async/module/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/oracle.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
prophet/dedup.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/prophet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
prophet/prophet.go (1)
Learnt from: Pitasi
PR: warden-protocol/wardenprotocol#1141
File: prophet/prophet.go:98-100
Timestamp: 2024-12-13T11:47:21.697Z
Learning: In the `prophet/prophet.go` file, for the implementation of the non-blocking queue `q`, prefer using a buffered channel that discards overflowing items instead of spawning a goroutine per item added, to prevent resource exhaustion.
🪛 golangci-lint (1.62.2)
prophet/prophet.go
90-90: Error return value of p.resultsWriter.Remove is not checked
(errcheck)
🪛 buf (1.47.2)
proto/warden/async/v1beta1/ve.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/warden/vemanager/vemanager.proto
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (15)
proto/warden/vemanager/vemanager.proto (2)
3-3: Package directory structure needs adjustment
The package warden.vemanager should be within directory warden/vemanager relative to root, but it's currently in proto/warden/vemanager.
🧰 Tools
🪛 buf (1.47.2)
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
7-12: LGTM! Well-documented message definition
The VoteExtensions message is well-designed with:
- Clear documentation explaining its purpose
- Appropriate use of repeated bytes for flexibility
- Simple, focused structure that aligns with ABCI requirements
proto/warden/async/v1beta1/ve.proto (1)
5-7: Verify gogoproto import availability
The static analysis indicates the gogoproto import might be missing.
✅ Verification successful
The gogoproto import is available and correctly referenced
The verification confirms that gogoproto/gogo.proto is available at wardenjs/proto/gogoproto/gogo.proto and is widely used across the codebase. The import statement in proto/warden/async/v1beta1/ve.proto is valid and correctly references the available protobuf definition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if gogoproto is available in the project
fd -t f "gogo.proto"
rg -l "gogoproto"
Length of output: 9069
🧰 Tools
🪛 buf (1.47.2)
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
prophet/dedup.go (1)
25-49: LGTM! Well-implemented deduplication with proper error handling
The implementation:
- Properly handles errors from LRU creation
- Safely manages goroutine lifecycle
- Correctly implements channel operations
prophet/prophet.go (2)
105-111: LGTM! Non-blocking queue implementation.
The implementation correctly uses a buffered channel with item dropping behavior when full, as previously discussed.
89-91:
Handle error from Remove operation.
The error returned by p.resultsWriter.Remove() should be handled to prevent potential issues from being silently ignored.
return values, func() {
- p.resultsWriter.Remove(values...)
+ if err := p.resultsWriter.Remove(values...); err != nil {
+ // Consider logging the error since this is in a callback
+ slog.Error("failed to remove results", "error", err)
+ }
}Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
90-90: Error return value of p.resultsWriter.Remove is not checked
(errcheck)
warden/x/async/module/module.go (1)
194-195: LGTM! Clean Prophet integration.
The Prophet integration is well-implemented as an optional dependency, following proper dependency injection patterns.
Also applies to: 217-217
warden/x/async/keeper/abci.go (1)
124-132:
Consider rejecting invalid async transactions consistently.
When async transaction unmarshalling fails, the handler accepts the proposal despite the comment suggesting rejection for consistency with slinky's behavior.
if err := tx.Unmarshal(asyncTx); err != nil {
log.Error("failed to unmarshal async tx", "err", err)
- return &cometabci.ResponseProcessProposal{
- Status: cometabci.ResponseProcessProposal_ACCEPT,
- }, nil
+ return &cometabci.ResponseProcessProposal{
+ Status: cometabci.ResponseProcessProposal_REJECT,
+ }, nil
}Likely invalid or redundant comment.
warden/app/oracle.go (5)
284-291: Prevent potential panic by checking Extensions slice length
The Decode method accesses w.Extensions[0] without checking if the slice is empty, which could cause a panic.
294-295: Ensure Encode method wraps the vote extension properly
The Encode method directly returns the encoded vote extension without wrapping it in a VoteExtensions message, which is inconsistent with the Decode method's behavior.
245-254: LGTM: Well-structured proposal handler combination
The implementation correctly handles sequential execution and error propagation of the proposal handlers.
125-130: LGTM: Proper codec initialization
The veCodec initialization correctly wraps the slinky codec with appropriate compression configuration.
224-242: Verify handler registration order
Please confirm if the order of handler registration (slinky before async) is significant for the vote extension processing flow.
✅ Verification successful
Order of handler registration is significant
The order of handler registration is important as it affects how the handlers are processed:
- For ExtendVoteHandler: Extensions are collected in registration order and combined into a single byte array
- For VerifyVoteExtensionHandler: Extensions are verified in the same order they were created, with a 1:1 mapping between extend and verify handlers
- For PrepareProposalHandler: Handlers form a pipeline where output from one handler becomes input for the next, making order crucial
The current registration order (slinky before async) should be maintained unless there's a specific requirement for async handlers to process before slinky handlers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any order dependencies in handler registration
# Look for comments or documentation about handler ordering
rg -A 5 "Register.*handler.*order"
Length of output: 36
Script:
#!/bin/bash
# Let's check the VoteExtensionManager implementation to understand how handlers are processed
ast-grep --pattern 'type VoteExtensionManager struct {
$$$
}'
# Also check the Register method implementation
ast-grep --pattern 'func ($_ *VoteExtensionManager) Register($$$) {
$$$
}'
# And the handler methods to see how they process multiple handlers
ast-grep --pattern 'func ($_ *VoteExtensionManager) ExtendVoteHandler($$$) {
$$$
}'
ast-grep --pattern 'func ($_ *VoteExtensionManager) VerifyVoteExtensionHandler($$$) {
$$$
}'
ast-grep --pattern 'func ($_ *VoteExtensionManager) PrepareProposalHandler($$$) {
$$$
}'
Length of output: 1600
Script:
#!/bin/bash
# Let's check how the handlers are executed to understand if order matters
ast-grep --pattern 'func ($_ *VoteExtensionManager) ExtendVoteHandler() sdk.ExtendVoteHandler {
$$$
}'
ast-grep --pattern 'func ($_ *VoteExtensionManager) VerifyVoteExtensionHandler() sdk.VerifyVoteExtensionHandler {
$$$
}'
ast-grep --pattern 'func ($_ *VoteExtensionManager) PrepareProposalHandler() sdk.PrepareProposalHandler {
$$$
}'
Length of output: 5043
CHANGELOG.md (1)
57-57: LGTM: Well-structured changelog entry
The prophet module introduction is properly documented with clear description of its features.
warden/app/app.go (1)
275-278: LGTM: Proper dependency injection configuration
The prophet instance is correctly supplied to the dependency injection container.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
prophet/prophet.go (1)
15-15: Useconstfor unchanging variablesThe variables
queueBufferSizeanddefaultSetSizeare constants and should be declared usingconstinstead ofvarto convey that their values do not change.Apply this diff to update the variable declarations:
- var queueBufferSize = 100 + const queueBufferSize = 100 ... - var defaultSetSize = 10000 + const defaultSetSize = 10000Also applies to: 117-117
proto/warden/async/v1beta1/ve.proto (1)
19-24: Enhance documentation for AsyncVoteExtensionConsider adding more detailed documentation about:
- The relationship between results and votes
- When this extension is used
- The expected format and constraints of the data
warden/x/async/keeper/keeper.go (1)
29-30: Add documentation for prophet fieldThe new field
p *prophet.Placks documentation explaining its purpose and usage within the Keeper struct.warden/x/async/keeper/abci_test.go (2)
9-73: Add test case for negative maxSizeBytesThe test cases are comprehensive but should also include a case for negative maxSizeBytes to ensure proper handling of invalid input.
tests := []struct { name string args args want [][]byte }{ + { + name: "negative max size", + args: args{ + txs: [][]byte{[]byte("tx1")}, + maxSizeBytes: -1, + }, + want: nil, + }, // existing test cases... }
75-137: Add test case for nil newTxConsider adding a test case to verify behavior when newTx is nil:
tests := []struct { name string args args want [][]byte }{ + { + name: "nil newTx", + args: args{ + newTx: nil, + position: 0, + appTxs: [][]byte{[]byte("appTx1")}, + }, + want: [][]byte{[]byte("appTx1")}, + }, // existing test cases... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/warden/async/v1beta1/ve.pulsar.gois excluded by!api/**api/warden/vemanager/vemanager.pulsar.gois excluded by!api/**warden/app/vemanager/vemanager.pb.gois excluded by!**/*.pb.gowarden/x/async/types/v1beta1/ve.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
CHANGELOG.md(1 hunks)go.mod(1 hunks)prophet/dedup.go(1 hunks)prophet/doc.go(1 hunks)prophet/exec.go(1 hunks)prophet/future.go(1 hunks)prophet/handlers.go(1 hunks)prophet/handlers/echo/echo.go(1 hunks)prophet/prophet.go(1 hunks)prophet/registry.go(1 hunks)proto/warden/async/v1beta1/ve.proto(1 hunks)proto/warden/vemanager/vemanager.proto(1 hunks)warden/app/app.go(6 hunks)warden/app/oracle.go(4 hunks)warden/app/vemanager/vemanager.go(1 hunks)warden/testutil/keeper/async.go(1 hunks)warden/x/async/keeper/abci.go(1 hunks)warden/x/async/keeper/abci_test.go(1 hunks)warden/x/async/keeper/futures.go(5 hunks)warden/x/async/keeper/keeper.go(5 hunks)warden/x/async/module/module.go(3 hunks)warden/x/async/types/v1beta1/future.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- prophet/doc.go
- warden/testutil/keeper/async.go
- warden/x/async/module/module.go
- warden/x/async/keeper/futures.go
- prophet/handlers.go
- go.mod
- prophet/registry.go
- prophet/exec.go
- prophet/future.go
- CHANGELOG.md
- prophet/dedup.go
- prophet/handlers/echo/echo.go
🧰 Additional context used
📓 Path-based instructions (8)
warden/x/async/types/v1beta1/future.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/abci_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
warden/app/oracle.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/vemanager/vemanager.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/prophet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
prophet/prophet.go (1)
Learnt from: Pitasi
PR: warden-protocol/wardenprotocol#1141
File: prophet/prophet.go:98-100
Timestamp: 2024-12-13T11:47:21.697Z
Learning: In the `prophet/prophet.go` file, for the implementation of the non-blocking queue `q`, prefer using a buffered channel that discards overflowing items instead of spawning a goroutine per item added, to prevent resource exhaustion.
🪛 buf (1.47.2)
proto/warden/async/v1beta1/ve.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/warden/vemanager/vemanager.proto
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (12)
prophet/prophet.go (1)
90-90:
Handle the error returned by p.resultsWriter.Remove(values...)
The error returned by Remove is not being checked. Ignoring errors can lead to unexpected behavior or hidden bugs. Please handle the error appropriately.
Apply this diff to handle the error:
return values, func() {
- p.resultsWriter.Remove(values...)
+ if err := p.resultsWriter.Remove(values...); err != nil {
+ // Handle the error appropriately, e.g., log it
+ slog.Error("Failed to remove results", "error", err)
+ }
}warden/x/async/keeper/abci.go (1)
125-133:
Consider rejecting the proposal when asyncTx fails to unmarshal
Currently, if asyncTx.Unmarshal fails, the proposal is still accepted. To ensure consistency and correctness, consider rejecting the proposal in this case.
Apply this diff to reject the proposal when unmarshalling fails:
if err := tx.Unmarshal(asyncTx); err != nil {
log.Error("failed to unmarshal async tx", "err", err)
// probably not an async tx?
// but slinky in this case rejects their proposal so maybe we
// should do the same?
- return &cometabci.ResponseProcessProposal{
- Status: cometabci.ResponseProcessProposal_ACCEPT,
- }, nil
+ return &cometabci.ResponseProcessProposal{
+ Status: cometabci.ResponseProcessProposal_REJECT,
+ }, nil
}warden/x/async/types/v1beta1/future.go (1)
3-6: Implementation of IsValid method is appropriate
The IsValid method correctly checks for valid FutureVoteType values and is implemented efficiently.
proto/warden/vemanager/vemanager.proto (1)
3-3:
Adjust package declaration or directory structure to match expected layout
The package warden.vemanager should be within a directory warden/vemanager relative to the root, but it is currently in proto/warden/vemanager. To adhere to best practices and avoid package resolution issues, adjust the package declaration or reorganize the directory structure accordingly.
Options to fix this issue:
- Option 1: Change the package declaration to
proto.warden.vemanagerto match the current directory structure. - Option 2: Move the file to the correct directory (
warden/vemanager) to align with the package declaration.
Ensure that the package and directory structure are consistent to prevent any import or compilation issues.
🧰 Tools
🪛 buf (1.47.2)
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
proto/warden/async/v1beta1/ve.proto (2)
14-16: Address data duplication with slinky
The FIXME comment indicates potential waste of block space due to data duplication. This should be addressed before merging.
26-34: Verify future_id validation
Ensure that future_id validation is implemented in the keeper layer to prevent invalid IDs from being stored.
warden/x/async/keeper/keeper.go (2)
94-108: Error handling could be improved in AddFutureResult
The method performs two operations (SetResult and SetFutureVote) but doesn't handle partial failures gracefully. If SetFutureVote fails, we might end up with inconsistent state.
110-115: LGTM! Proper validation implemented
The implementation correctly validates the vote type before storing and uses appropriate key composition.
warden/app/vemanager/vemanager.go (2)
17-21: LGTM! Well-structured manager type.
The VoteExtensionManager struct is well-designed with clear separation of handler types.
77-79: 🛠️ Refactor suggestion
Consider adding equality check for handler lengths.
The current check only verifies if there are too many extensions, but should also verify if there are too few.
Apply this diff to add equality check:
- if len(w.Extensions) > len(m.verifyVoteExtensionHandler) {
- return nil, fmt.Errorf("too many vote extensions, not enough VerifyVoteExtensionHandlers registered to the VoteExtensionManager")
+ if len(w.Extensions) != len(m.verifyVoteExtensionHandler) {
+ return nil, fmt.Errorf("mismatched number of vote extensions (%d) and handlers (%d)",
+ len(w.Extensions), len(m.verifyVoteExtensionHandler))
}Likely invalid or redundant comment.
warden/app/oracle.go (1)
299-300:
Encode method needs to wrap vote extension.
The Encode method should wrap the vote extension in a VoteExtensions message for consistency with Decode.
warden/app/app.go (1)
248-251:
Prophet instance requires cleanup mechanism.
The prophet package lacks a shutdown/cleanup method for proper resource management.
Also applies to: 406-408
The role of the vemanager is to provide an abstraction level for Vote Extensions, that are usually treated as []byte. A VoteExtensionManager acts similarly to a router for Vote Extensions, where multiple ABCI handlers can be registered, and each handler will only see it's own view, without having to know about vemanager at all.
WardenSlinkyCodec make slinky's vote extension compatible with vemanager. The vote extension by slinky is now being wrapped into vemanager.VoteExtensions, this codec allows to encode/decode a vemanager.VoteExtensions into the normal slinky's struct.
App now starts a prophet.P and wires it into x/async keeper. x/async provides basic ABCI handlers that are registered into the existing vemanager.
Implement an EndBlocker that takes all the Futures that do not have a result yet, and schedule them to the prophet's process for asynchronous execution.
…dler ExtendVoteHandler: collect Futures' results from Prophet, and build the x/async vote extension with them. VerifyVoteExtensionHandler: verify that the incoming vote extension bytes is a valid protobuf message.
PrepareProposal injects an AsyncInjectedTx in the block, right after the slinky transaction, i.e. the tx #0 is slinky, the tx #1 is x/async, and the rest are the normal transactions of the block. ProcessProposal checks if the transactions bytes in position #1 in the proposal are a valid AsyncInjectedTx.
PreBlocker takes the tx #1 of the block, unmarshal the AsyncInjectedTx and executes it. The AsyncInjectedTx includes results and votes for futures, coming from different validators. The PreBlocker updates the local application state accordingly.
|
I moved the "hidden" |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
prophet/prophet.go (1)
80-92:⚠️ Potential issue[Duplicate Comment] Handle error returned by p.resultsWriter.Remove().
This is similar to a previous comment. Not checking the error can hide potential I/O or state issues. Even if it's usually non-fatal, logging or handling that error can aid debugging.
return values, func() { - p.resultsWriter.Remove(values...) + if err := p.resultsWriter.Remove(values...); err != nil { + slog.Error("Error removing results", "error", err) + } }prophet/exec.go (2)
17-39:⚠️ Potential issueCritical: Improve goroutine management and error handling
The ExecFutures function needs significant improvements in error handling and goroutine management.
Building upon previous review comments, also add graceful shutdown:
-func ExecFutures(r FutureReader, w FutureResultWriter) error { +func ExecFutures(ctx context.Context, r FutureReader, w FutureResultWriter) error { log := slog.With("process", "exec_futures") + errCh := make(chan error, 1) + doneCh := make(chan struct{}) go func() { + defer close(doneCh) + defer close(errCh) - for future := range r.Read() { + for future := range r.Read(ctx) { + select { + case <-ctx.Done(): + errCh <- ctx.Err() + return + default: log := log.With("future", future.ID) log.Debug("running future") - output, err := Execute(context.TODO(), future) + output, err := Execute(ctx, future) if err != nil { log.Error("failed to run future", "err", err) + errCh <- fmt.Errorf("executing future %s: %w", future.ID, err) continue } err = w.Write(output) if err != nil { log.Error("failed to write future result", "err", err) + errCh <- fmt.Errorf("writing future result %s: %w", future.ID, err) continue } + } } }() - return nil + select { + case err := <-errCh: + return err + case <-doneCh: + return nil + } }
50-70:⚠️ Potential issueCritical: Apply similar improvements to ExecVotes
The ExecVotes function has the same issues as ExecFutures.
Apply similar improvements:
-func ExecVotes(r FutureResultReader, w VoteWriter) error { +func ExecVotes(ctx context.Context, r FutureResultReader, w VoteWriter) error { log := slog.With("process", "exec_votes") + errCh := make(chan error, 1) + doneCh := make(chan struct{}) go func() { + defer close(doneCh) + defer close(errCh) - for proposal := range r.Read() { + for proposal := range r.Read(ctx) { + select { + case <-ctx.Done(): + errCh <- ctx.Err() + return + default: plog := log.With("future", proposal.ID) plog.Debug("verifying future proposal") - err := Verify(context.TODO(), proposal) + verifyErr := Verify(ctx, proposal) - if err := w.Write(Vote{ + if writeErr := w.Write(Vote{ ID: proposal.ID, - Err: err, + Err: verifyErr, - }); err != nil { + }); writeErr != nil { plog.Error("failed to write vote", + "verify_err", verifyErr, + "write_err", writeErr) + errCh <- fmt.Errorf("writing vote for %s: %w", proposal.ID, writeErr) continue } + } } }() - return nil + select { + case err := <-errCh: + return err + case <-doneCh: + return nil + } }
🧹 Nitpick comments (8)
prophet/prophet.go (1)
15-15: Clarify buffer size usage.Consider exposing or documenting the “queueBufferSize” constant in a config file or environment variable to allow easier fine-tuning for different deployment scenarios where traffic volume may vary.
warden/app/app.go (1)
407-409: No mechanism to stop prophet gracefully.Calling app.prophet.Run() is fine, but consider the future possibility of a shutdown or cancellation context, especially if the underlying SDK’s shutdown signals become available.
warden/x/async/keeper/futures.go (1)
90-116: Consider batch processing for pending futures retrievalWhile the current implementation with pagination is functional, it could be optimized to reduce database operations.
Consider using batch operations to reduce the number of database calls:
-func (k *FutureKeeper) PendingFutures(ctx context.Context, limit int) ([]types.Future, error) { +func (k *FutureKeeper) PendingFutures(ctx context.Context, limit int) ([]types.Future, error) { it, err := k.pendingFutures.IterateRaw(ctx, nil, nil, collections.OrderAscending) if err != nil { return nil, err } defer it.Close() - futures := make([]types.Future, 0, limit) + ids := make([]uint64, 0, limit) for ; it.Valid(); it.Next() { id, err := it.Key() if err != nil { return nil, err } + ids = append(ids, id) + if len(ids) == limit { + break + } + } - fut, err := k.futures.Get(ctx, id) - if err != nil { - return nil, err - } - - futures = append(futures, fut) - if len(futures) == limit { - break - } + // Batch fetch futures + futures := make([]types.Future, 0, len(ids)) + for _, id := range ids { + future, err := k.futures.Get(ctx, id) + if err != nil { + return nil, err + } + futures = append(futures, future) } return futures, nil }warden/x/async/keeper/abci.go (3)
25-25: Consider making the futures limit configurableThe hardcoded limit of 10 futures per block might need adjustment based on network conditions and performance requirements.
- futures, err := k.futures.PendingFutures(ctx, 10) + futures, err := k.futures.PendingFutures(ctx, k.config.MaxFuturesPerBlock)
71-77: Enhance error logging in VerifyVoteExtensionHandlerWhen unmarshaling fails, the error details are silently discarded. Consider logging the error for better debugging.
err := ve.Unmarshal(req.VoteExtension) if err != nil { + ctx.Logger().Debug("failed to unmarshal vote extension", "err", err) return &cometabci.ResponseVerifyVoteExtension{ Status: cometabci.ResponseVerifyVoteExtension_REJECT, }, nil }
171-171: Address the TODO comment about VE signature verificationThe comment indicates that vote extension signature verification is pending implementation. This is a security-critical feature that should be implemented.
Would you like me to help implement the signature verification logic or create a GitHub issue to track this task?
warden/app/oracle.go (1)
215-223: Consider adding error recovery for combined handlersThe
combineProcessProposalandcombinePreBlockerfunctions could benefit from panic recovery to prevent cascading failures.func combineProcessProposal(a, b sdk.ProcessProposalHandler) sdk.ProcessProposalHandler { return func(ctx sdk.Context, req *cometabci.RequestProcessProposal) (*cometabci.ResponseProcessProposal, error) { + defer func() { + if r := recover(); r != nil { + ctx.Logger().Error("recovered from panic in process proposal", "panic", r) + } + }() resp, err := a(ctx, req) if err != nil || resp.Status == cometabci.ResponseProcessProposal_REJECT { return resp, err } return b(ctx, req) } }prophet/handlers/echo/echo.go (1)
17-19: Consider adding basic input validationWhile the echo handler is simple, it would be good practice to validate the input.
Consider adding:
func (s Handler) Execute(ctx context.Context, input []byte) ([]byte, error) { + if len(input) == 0 { + return nil, fmt.Errorf("empty input") + } return input, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
api/warden/async/v1beta1/ve.pulsar.gois excluded by!api/**api/warden/vemanager/vemanager.pulsar.gois excluded by!api/**warden/app/vemanager/vemanager.pb.gois excluded by!**/*.pb.gowarden/x/async/types/v1beta1/ve.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
CHANGELOG.md(1 hunks)go.mod(1 hunks)prophet/dedup.go(1 hunks)prophet/doc.go(1 hunks)prophet/exec.go(1 hunks)prophet/future.go(1 hunks)prophet/handlers.go(1 hunks)prophet/handlers/echo/echo.go(1 hunks)prophet/prophet.go(1 hunks)prophet/registry.go(1 hunks)proto/warden/async/v1beta1/ve.proto(1 hunks)proto/warden/vemanager/vemanager.proto(1 hunks)warden/app/app.go(6 hunks)warden/app/oracle.go(4 hunks)warden/app/vemanager/vemanager.go(1 hunks)warden/testutil/keeper/async.go(1 hunks)warden/x/async/keeper/abci.go(1 hunks)warden/x/async/keeper/abci_test.go(1 hunks)warden/x/async/keeper/futures.go(5 hunks)warden/x/async/keeper/keeper.go(5 hunks)warden/x/async/module/module.go(3 hunks)warden/x/async/types/v1beta1/future.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- warden/x/async/types/v1beta1/future.go
- prophet/doc.go
- prophet/registry.go
- warden/x/async/keeper/abci_test.go
- go.mod
- warden/x/async/module/module.go
- prophet/dedup.go
- prophet/future.go
- warden/testutil/keeper/async.go
- CHANGELOG.md
- warden/app/vemanager/vemanager.go
- warden/x/async/keeper/keeper.go
🧰 Additional context used
📓 Path-based instructions (8)
warden/x/async/keeper/futures.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/handlers/echo/echo.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/exec.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/oracle.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/handlers.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
prophet/prophet.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/app/app.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
warden/x/async/keeper/abci.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (2)
prophet/prophet.go (1)
Learnt from: Pitasi
PR: warden-protocol/wardenprotocol#1141
File: prophet/prophet.go:98-100
Timestamp: 2024-12-13T11:47:21.697Z
Learning: In the `prophet/prophet.go` file, for the implementation of the non-blocking queue `q`, prefer using a buffered channel that discards overflowing items instead of spawning a goroutine per item added, to prevent resource exhaustion.
warden/app/app.go (1)
Learnt from: Pitasi
PR: warden-protocol/wardenprotocol#1141
File: warden/app/app.go:248-251
Timestamp: 2024-12-13T15:13:03.523Z
Learning: The Cosmos SDK does not provide an easy way to be notified of shutdown, making it challenging to implement cleanup mechanisms for resources like the Prophet instance.
🪛 buf (1.47.2)
proto/warden/async/v1beta1/ve.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
proto/warden/vemanager/vemanager.proto
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
🔇 Additional comments (18)
prophet/prophet.go (2)
51-65: Lack of graceful shutdown mechanism.
The Run method starts background loops for futures and votes without a way to signal them to stop. If the application terminates abruptly, unprocessed items could be lost. Consider adding a shutdown or context-based cancellation approach in the future if the underlying framework allows.
105-111: Implementation aligns with learning to prevent resource exhaustion.
The non-blocking send with a buffered channel addresses previous concerns about spawning a goroutine per item. It provides a balance between concurrency and the risk of unbounded goroutines.
warden/app/app.go (1)
248-252: Registering the echo handler is successful.
Looks good. The approach of registering the "echo" handler with prophet before building the app is straightforward.
proto/warden/vemanager/vemanager.proto (2)
3-3: Package location needs adjustment
Files with package "warden.vemanager" must be within directory "warden/vemanager" relative to root.
🧰 Tools
🪛 buf (1.47.2)
3-3: Files with package "warden.vemanager" must be within a directory "warden/vemanager" relative to root but were in directory "proto/warden/vemanager".
(PACKAGE_DIRECTORY_MATCH)
7-12: Well-structured message definition with clear documentation
The VoteExtensions message is well-documented and correctly implements the requirement to distinguish between multiple extensions from different sources.
proto/warden/async/v1beta1/ve.proto (3)
14-16: Address data duplication with slinky component
The FIXME comment indicates potential block space waste due to data duplication.
19-34: Well-structured message hierarchy for async vote extensions
The message definitions (AsyncVoteExtension, VEResultItem, VEVoteItem) form a clear and logical hierarchy for managing async votes and results.
5-7: Verify proto imports availability
Please ensure the imported proto files are available in the build environment.
✅ Verification successful
All required proto imports are available
The verification confirms that all three required proto files are present in the repository:
gogoproto/gogo.protoatwardenjs/proto/gogoproto/gogo.prototendermint/abci/types.protoatwardenjs/proto/tendermint/abci/types.protowarden/async/v1beta1/future.protoatproto/warden/async/v1beta1/future.proto
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if required proto files exist in the repository
fd -e proto -x echo "Checking {}..." \; -x cat {} \; | grep -A 1 "^Checking.*\(gogo\|types\|future\)\.proto"
Length of output: 762
🧰 Tools
🪛 buf (1.47.2)
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
warden/x/async/keeper/futures.go (3)
19-19: Clean implementation of pending futures tracking
The addition of pendingFutures KeySet and its initialization in the keeper constructor is well-structured and follows the existing patterns.
Also applies to: 31-37
56-59: Good error handling in Append method
The implementation correctly handles the pending futures tracking while maintaining proper error handling.
72-74: Clean implementation of result setting
The removal of futures from pending set when setting results is implemented correctly.
warden/x/async/keeper/abci.go (2)
125-133: Consider rejecting the proposal when asyncTx fails to unmarshal
The code logs an error but still accepts the proposal. As the comment suggests that similar cases result in rejection, it would be consistent to reject the proposal here.
220-223: Consider error handling in panic case
The injectTx function uses panic for input validation. This could be dangerous in a production environment.
warden/app/oracle.go (1)
299-300: Ensure Encode method wraps the vote extension properly
The current implementation doesn't wrap the encoded vote extension in a VoteExtensions message, which is inconsistent with the Decode method's expectations.
prophet/handlers/echo/echo.go (3)
1-11: LGTM: Well-documented package with clear purpose
The package documentation clearly describes its testing purpose and the imports are properly organized.
21-26: LGTM: Proper verification implementation
The Verify method correctly implements the comparison logic using bytes.Compare.
13-15:
Add registration of the handler
The handler needs to be registered to be discoverable by the prophet package.
Add the following code:
type Handler struct{}
var _ prophet.FutureHandler = (*Handler)(nil)
+func init() {
+ if err := prophet.Register("echo", Handler{}); err != nil {
+ panic(fmt.Sprintf("failed to register echo handler: %v", err))
+ }
+}Likely invalid or redundant comment.
prophet/handlers.go (1)
10-19: LGTM: Well-defined interface with clear documentation
The FutureHandler interface is properly documented with clear method descriptions.
| // Verify verifies a given future result, by invoking the registered handler. | ||
| func Verify(ctx context.Context, f FutureResult) error { | ||
| s := getHandler(f.Handler) | ||
| if s == nil { | ||
| return fmt.Errorf("no future handler registered for %s", f.Handler) | ||
| } | ||
|
|
||
| log := slog.With("task", "Verify", "future", f.ID, "handler", f.Handler) | ||
| log.Debug("start") | ||
| start := time.Now() | ||
|
|
||
| err := s.Verify(ctx, f.Input, f.Output) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| log.Debug("end", "took", time.Since(start)) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add similar improvements to Verify function
The Verify function should have consistent error handling with Execute.
Apply similar improvements:
func Verify(ctx context.Context, f FutureResult) error {
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+
s := getHandler(f.Handler)
if s == nil {
- return fmt.Errorf("no future handler registered for %s", f.Handler)
+ return fmt.Errorf("no future handler registered for %s (id: %s)", f.Handler, f.ID)
}Committable suggestion skipped: line range outside the PR's diff.
| // Execute executes a given future, by invoking the registered handler. | ||
| func Execute(ctx context.Context, f Future) (FutureResult, error) { | ||
| s := getHandler(f.Handler) | ||
| if s == nil { | ||
| return FutureResult{}, fmt.Errorf("no future handler registered for %s", f.Handler) | ||
| } | ||
|
|
||
| log := slog.With("task", "Execute", "future", f.ID, "handler", f.Handler) | ||
| log.Debug("start") | ||
| start := time.Now() | ||
|
|
||
| output, err := s.Execute(ctx, f.Input) | ||
| if err != nil { | ||
| return FutureResult{}, fmt.Errorf("executing future: %w", err) | ||
| } | ||
|
|
||
| log.Debug("end", "took", time.Since(start)) | ||
|
|
||
| return FutureResult{ | ||
| Future: f, | ||
| Output: output, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add context timeout and improve error handling
The Execute function should have timeout protection and more detailed error context.
Consider these improvements:
func Execute(ctx context.Context, f Future) (FutureResult, error) {
+ // Add timeout protection
+ ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+
s := getHandler(f.Handler)
if s == nil {
- return FutureResult{}, fmt.Errorf("no future handler registered for %s", f.Handler)
+ return FutureResult{}, fmt.Errorf("no future handler registered for %s (id: %s)", f.Handler, f.ID)
}
log := slog.With("task", "Execute", "future", f.ID, "handler", f.Handler)
log.Debug("start")
start := time.Now()
output, err := s.Execute(ctx, f.Input)
if err != nil {
- return FutureResult{}, fmt.Errorf("executing future: %w", err)
+ return FutureResult{}, fmt.Errorf("executing future %s with handler %s: %w", f.ID, f.Handler, err)
}Committable suggestion skipped: line range outside the PR's diff.
This PR introduces Prophet.
My suggestion to review the changes:
prophet/doc.gofor an intro to prophetecho/echo.gofor an example future handlerx/async/keeper/abci.go, the rest is wiring together everythingmake sure to check the individual commits in order, and read the descriptions, i tried to order them in a way that makes sense
Summary by CodeRabbit
New Features
Consensus Breaking Changes
Bug Fixes
Documentation
prophetpackage.Tests