(DAL) Data Availability Layer Beta #1720
Conversation
|
Warning Rate limit exceeded@nick-bisonai has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 24 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThis update introduces improved testing functionality and a new GitHub Actions workflow for the Data Access Layer (DAL) component. Enhancements include testing for the collector, API controller, and various database operations, setup and cleanup routines, and the addition of new constants and functions to the collector. The new CI workflow tests DAL functionality with PostgreSQL and Redis, ensuring code quality and automated tests. Changes
Poem
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 review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.work.sumis excluded by!**/*.sum,!**/*.sumnode/go.modis excluded by!**/*.modnode/go.sumis excluded by!**/*.sum,!**/*.sumnode/taskfiles/taskfile.local.ymlis excluded by!**/*.yml
Files selected for processing (14)
- node/cmd/dal/main.go (1 hunks)
- node/pkg/chain/websocketchainreader/type.go (1 hunks)
- node/pkg/chain/websocketchainreader/websocketreader.go (1 hunks)
- node/pkg/dalapi/api/controller.go (1 hunks)
- node/pkg/dalapi/api/route.go (1 hunks)
- node/pkg/dalapi/app.go (1 hunks)
- node/pkg/dalapi/collector/collector.go (1 hunks)
- node/pkg/dalapi/collector/utils.go (1 hunks)
- node/pkg/dalapi/common/types.go (1 hunks)
- node/pkg/dalapi/utils/utils.go (1 hunks)
- node/pkg/error/sentinel.go (1 hunks)
- node/pkg/websocketfetcher/app.go (1 hunks)
- node/script/test_websocketDexFetcher/main.go (1 hunks)
- node/script/test_websocketchainreader/main.go (1 hunks)
Files not reviewed due to errors (1)
- node/pkg/error/sentinel.go (no review received)
Files skipped from review due to trivial changes (2)
- node/cmd/dal/main.go
- node/pkg/dalapi/common/types.go
Additional comments not posted (41)
node/pkg/dalapi/api/route.go (2)
13-13: Verify WebSocket route setup.Ensure that the WebSocket route is correctly configured and tested.
Do you have tests verifying the WebSocket connection and data flow?
8-14: LGTM!The route definitions are clear and well-structured.
node/pkg/dalapi/app.go (2)
18-19: Good error handling practice.Logging the error before returning is a good practice for debugging.
22-26: Ensure background tasks are properly managed.Verify that the background tasks managed by
ApiController.Runare properly started and stopped, especially during shutdown.Do you have a mechanism to gracefully stop background tasks during server shutdown?
node/pkg/chain/websocketchainreader/type.go (6)
18-22: LGTM!The struct
ChainReaderConfigis well-defined and the field names are clear.
Line range hint
43-49: LGTM!The struct
ChainReaderis well-defined and the field names are clear.
26-30: LGTM!The function
WithKaiaWebsocketUrlis well-defined and the function name is clear.
32-36: LGTM!The function
WithEthWebsocketUrlis well-defined and the function name is clear.
38-42: LGTM!The function
WithRetryIntervalis well-defined and the function name is clear.
41-42: LGTM!The function
WithStartBlockNumberis well-defined and the function name is clear.node/script/test_websocketchainreader/main.go (2)
30-33: LGTM!The code for creating the
websocketchainreaderwith the provided WebSocket URLs is clear and well-structured.
Line range hint
37-38: Good practice: Closing the channel.Closing the channel after use is a good practice to avoid memory leaks.
node/pkg/dalapi/utils/utils.go (1)
78-97: LGTM!The code changes are approved.
node/pkg/dalapi/collector/utils.go (3)
17-29: Enhance error handling with descriptive messages.The error message "failed to cast result to []interface{}" can be more descriptive to include the context of the operation.
- return nil, errors.New("failed to cast result to []interface{}") + return nil, fmt.Errorf("failed to cast result to []interface{} for contract address %s", submissionProxyContractAddr)
31-67: Enhance error handling with descriptive messages.The error messages can be more descriptive to include the context of the operation.
- return eventParseErr + return fmt.Errorf("failed to parse method signature for OracleAdded event: %w", eventParseErr)- return err + return fmt.Errorf("failed to generate event ABI for OracleAdded event: %w", err)
70-89: LGTM!The code changes are approved.
node/script/test_websocketDexFetcher/main.go (1)
107-110: Improve error handling granularity.The error handling can be more granular to provide specific error messages for each operation.
chainReader, err := websocketchainreader.New( websocketchainreader.WithEthWebsocketUrl(ethWebsocketUrl), websocketchainreader.WithKaiaWebsocketUrl(kaiaWebsocketUrl), ) if err != nil { log.Error().Err(err).Msg("failed to create websocketchainreader with provided URLs") return }node/pkg/dalapi/collector/collector.go (9)
84-96: LGTM!The
Startfunction is well-implemented and handles initialization and context setup correctly.
98-101: LGTM!The
Stopfunction correctly handles stopping the collector.
104-107: LGTM!The
receivefunction correctly starts goroutines for each config ID.
110-122: LGTM!The
receiveEachfunction correctly subscribes to the submission stream and processes incoming data.
125-133: LGTM!The
processIncomingDatafunction correctly processes incoming data and handles errors.
135-167: LGTM!The
IncomingDataToOutgoingDatafunction correctly converts data and handles proof ordering, including refreshing the whitelist if necessary.
170-191: LGTM!The
trackOracleAddedfunction correctly subscribes to the oracle added event and updates the whitelist.
59-62: EnsuregetAllOraclesis implemented correctly.The function
getAllOraclesis not provided in the current context. Ensure it is implemented correctly and used appropriately.
139-145: EnsureorderProofis implemented correctly.The function
orderProofis not provided in the current context. Ensure it is implemented correctly and used appropriately.node/pkg/websocketfetcher/app.go (6)
Line range hint
55-57: LGTM!The
Newfunction correctly initializes a newAppinstance.
Line range hint
59-78: LGTM!The
Initfunction correctly sets up necessary configurations and initializes CEX and DEX fetchers.
Line range hint
80-110: LGTM!The
initializeCexfunction correctly initializes CEX fetchers and handles errors appropriately.
Line range hint
201-223: LGTM!The
Startfunction correctly starts the app and its fetchers, handling the context and ticker appropriately.
Line range hint
225-247: LGTM!The
storeFeedDatafunction correctly stores feed data from the buffer and handles errors appropriately.
197-199: LGTM!The
getEnvOrDefaultfunction correctly retrieves environment variables with a default value fallback.node/pkg/dalapi/api/controller.go (9)
35-44: LGTM!The
NewControllerfunction correctly initializes a newControllerinstance.
47-87: LGTM!The
getLatestSubmissionDatafunction correctly retrieves the latest submission data and handles errors appropriately.
89-108: LGTM!The
getLatestSubmissionDataSinglefunction correctly retrieves the latest submission data for a single symbol and handles errors appropriately.
111-137: LGTM!The
handleWebsocketfunction correctly handles websocket connections and subscriptions, including error handling.
140-146: LGTM!The
configIdToSymbolfunction correctly converts a config ID to a symbol.
149-158: LGTM!The
broadcastDataForSymbolfunction correctly broadcasts data for a specific symbol to subscribed clients and handles errors appropriately.
161-187: LGTM!The
Runfunction correctly starts the controller and its collector, handling the context and channels appropriately.
190-209: LGTM!The
initfunction correctly initializes the API controller and handles errors appropriately.
211-227: LGTM!The
getLatestFeedsfunction correctly retrieves the latest feeds and converts them to outgoing data, handling errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 24
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.work.sumis excluded by!**/*.sum,!**/*.sumnode/go.modis excluded by!**/*.modnode/go.sumis excluded by!**/*.sum,!**/*.sumnode/taskfiles/taskfile.local.ymlis excluded by!**/*.yml
Files selected for processing (14)
- node/.env.example (1 hunks)
- node/cmd/dal/main.go (1 hunks)
- node/pkg/chain/websocketchainreader/websocketreader.go (1 hunks)
- node/pkg/dal/api/controller.go (1 hunks)
- node/pkg/dal/api/route.go (1 hunks)
- node/pkg/dal/api/types.go (1 hunks)
- node/pkg/dal/app.go (1 hunks)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/collector/utils.go (1 hunks)
- node/pkg/dal/common/types.go (1 hunks)
- node/pkg/dal/tests/api_test.go (1 hunks)
- node/pkg/dal/tests/collector_test.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
- node/pkg/dal/utils/utils.go (1 hunks)
Files skipped from review due to trivial changes (2)
- node/.env.example
- node/cmd/dal/main.go
Additional comments not posted (20)
node/pkg/dal/common/types.go (1)
3-10: LGTM! Verify the consistency of data types.The struct definition looks good.
However, ensure that the data types (
stringforValueandAggregateTime,[]byteforProofandFeedHash) are consistent with the rest of the codebase and the expected data format.node/pkg/dal/api/route.go (1)
8-13: LGTM! Verify the correctness of WebSocket integration.The route definitions look good.
However, ensure that the WebSocket integration (
websocket.New(ApiController.handleWebsocket)) is correctly implemented and handles edge cases such as connection errors and client disconnections.node/pkg/dal/api/types.go (2)
10-13: LGTM!The struct definition looks good.
15-23: LGTM! Verify the consistency of data types.The struct definition looks good.
However, ensure that the data types (
map[*websocket.Conn]map[string]boolforclients,chan *websocket.Connforregisterandunregister, andmap[string]chan dalcommon.OutgoingSubmissionDataforbroadcast) are consistent with the rest of the codebase and the expected data format.node/pkg/dal/app.go (1)
14-38: LGTM! Verify the correctness of error handling and environment variable usage.The function implementation looks good.
However, ensure that the error handling is comprehensive and that the environment variable
DAL_API_PORTis correctly set and used.node/pkg/dal/tests/collector_test.go (2)
13-33: LGTM! Verify the comprehensiveness of assertions and cleanup.The test implementation looks good.
However, ensure that the assertions are comprehensive and that the cleanup is properly handled.
35-79: LGTM! Verify the comprehensiveness of assertions and cleanup.The test implementation looks good.
However, ensure that the assertions are comprehensive and that the cleanup is properly handled.
node/pkg/dal/utils/utils.go (1)
1-1: Ensure package name consistency.Ensure the package name
utilsis consistent with other utility files to maintain a standard naming convention.node/pkg/dal/collector/utils.go (1)
1-1: Ensure package name consistency.Ensure the package name
collectoris consistent with other collector files to maintain a standard naming convention.node/pkg/dal/tests/main_test.go (1)
1-1: Ensure package name consistency.Ensure the package name
testis consistent with other test files to maintain a standard naming convention.node/pkg/dal/tests/api_test.go (1)
1-1: Ensure package name consistency.Ensure the package name
testis consistent with other test files to maintain a standard naming convention.node/pkg/dal/collector/collector.go (4)
99-104: LGTM!The
Stopmethod correctly handles stopping the collector.
112-124: LGTM!The
receiveEachmethod correctly handles data reception and processing.
127-135: LGTM!The
processIncomingDatamethod correctly handles data processing and sending to the outgoing stream.
172-194: LGTM!The
trackOracleAddedmethod correctly handles event subscription and whitelist updating.node/pkg/dal/api/controller.go (5)
40-50: LGTM!The
NewControllerfunction correctly initializes a newControllerinstance.
77-84: LGTM!The
configIdToSymbolmethod correctly maps a configuration ID to its corresponding symbol.
100-127: LGTM!The
handleWebsocketmethod correctly handles WebSocket connections and subscription management.
129-169: LGTM!The
getLatestSubmissionDatamethod correctly retrieves the latest submission data for all configurations.
171-191: LGTM!The
getLatestSubmissionDataSinglemethod correctly retrieves the latest submission data for a specific symbol.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- node/.env.example (1 hunks)
- node/pkg/dal/api/controller.go (1 hunks)
- node/pkg/dal/app.go (1 hunks)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/collector/utils.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
- node/pkg/dal/utils/utils.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- node/.env.example
- node/pkg/dal/api/controller.go
- node/pkg/dal/app.go
- node/pkg/dal/collector/utils.go
- node/pkg/dal/tests/main_test.go
- node/pkg/dal/utils/utils.go
Additional comments not posted (8)
node/pkg/dal/collector/collector.go (8)
22-25: LGTM! Constants are appropriately defined.The constants
DefaultDecimals,GetAllOracles, andOracleAddedare straightforward and correctly defined.
28-42: LGTM! Struct fields are well-defined.The
Collectorstruct fields are appropriately defined to manage data streams, symbols, feed hashes, cached whitelist, context, cancellation function, and synchronization.
99-104: LGTM! Function is correctly implemented.The
Stopfunction is straightforward and correctly implemented to cancel the context and set it to nil.
106-114: Add error handling or logging for goroutines.Consider adding error handling or logging for the goroutines to ensure any issues are captured.
- go c.receiveEach(ctx, id) + go func(id int32) { + if err := c.receiveEach(ctx, id); err != nil { + log.Error().Err(err).Str("Player", "DalCollector").Msg("Error in receiveEach goroutine") + } + }(id)Likely invalid or redundant comment.
141-174: Add more context to error logs.Consider adding more context to error logs for better debugging.
- log.Error().Err(err).Str("Player", "DalCollector").Msg("failed to order proof") + log.Error().Err(err).Str("Player", "DalCollector").Str("Symbol", c.Symbols[data.GlobalAggregate.ConfigID]).Msg("failed to order proof") - log.Error().Err(getAllOraclesErr).Str("Player", "DalCollector").Msg("failed to refresh oracles") + log.Error().Err(getAllOraclesErr).Str("Player", "DalCollector").Str("Symbol", c.Symbols[data.GlobalAggregate.ConfigID]).Msg("failed to refresh oracles")Likely invalid or redundant comment.
176-198: LGTM! Verify error handling insubscribeAddOracleEvent.The
trackOracleAddedfunction is correctly implemented to subscribe to the oracle added event and update the cached whitelist. Ensure that thesubscribeAddOracleEventmethod handles errors appropriately.Verification successful
Verified: Error handling in
subscribeAddOracleEventis appropriate.The
subscribeAddOracleEventfunction correctly handles errors by returning them if the subscription fails. No further action is needed.
node/pkg/dal/collector/utils.go: Proper error handling insubscribeAddOracleEvent.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `subscribeAddOracleEvent`. # Test: Search for the `subscribeAddOracleEvent` function. Expect: Proper error handling. rg --type go --pattern 'func subscribeAddOracleEvent' --context 10Length of output: 480
Script:
#!/bin/bash # Description: Verify error handling in `subscribeAddOracleEvent`. # Test: Search for the `subscribeAddOracleEvent` function. Expect: Proper error handling. rg 'func subscribeAddOracleEvent' --type go -C 10Length of output: 1360
116-128: LGTM! Verify error handling indb.Subscribe.The
receiveEachfunction is correctly implemented to subscribe to a data stream and process incoming data. Ensure that thedb.Subscribemethod handles errors appropriately.Verification successful
LGTM! Verify error handling in
db.Subscribe.The
receiveEachfunction is correctly implemented to subscribe to a data stream and process incoming data. Thedb.Subscribemethod handles errors appropriately, including empty channels, unmarshalling errors, and retryable errors throughexecuteWithRetry.
node/pkg/db/redis.go: Comprehensive error handling indb.Subscribe.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in `db.Subscribe`. # Test: Search for the `Subscribe` method in the `db` package. Expect: Proper error handling. rg --type go --pattern 'func Subscribe' --context 10Length of output: 466
Script:
#!/bin/bash # Description: Verify error handling in `db.Subscribe`. # Corrected Test: Search for the `Subscribe` method in the `db` package. Expect: Proper error handling. rg 'func Subscribe' --type go --context 10Length of output: 1047
Script:
#!/bin/bash # Description: Verify comprehensive error handling in `db.Subscribe`. # Test: Display the full implementation of the `Subscribe` method to review all error handling cases. rg 'func Subscribe' --type go --context 30Length of output: 2731
131-139: LGTM! Verify error handling inIncomingDataToOutgoingData.The
processIncomingDatafunction is correctly implemented to convert incoming data to outgoing data and send it to the outgoing stream. Ensure that theIncomingDataToOutgoingDatamethod handles errors appropriately.
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (9)
node/script/test_dal_consumer/main.go (6)
29-33: Improve error logging for chain helper initialization.The error log message can be more descriptive to aid in debugging.
- log.Error().Err(err).Msg("NewTxHelper") + log.Error().Err(err).Str("Player", "TestConsumer").Msg("failed to create chain helper")
35-39: Improve error logging for data feed request.The error log message can be more descriptive to aid in debugging.
- log.Error().Err(err).Str("Player", "TestConsumer").Msg("failed to get data feed") + log.Error().Err(err).Str("Player", "TestConsumer").Str("URL", url).Msg("failed to get data feed")
42-46: Improve error logging for string to big.Int conversion.The error log message can be more descriptive to aid in debugging.
- log.Error().Str("Player", "TestConsumer").Msg("failed to convert string to big int") + log.Error().Str("Player", "TestConsumer").Str("Value", result.Value).Msg("failed to convert string to big int")
49-53: Improve error logging for string to big.Int conversion.The error log message can be more descriptive to aid in debugging.
- log.Error().Str("Player", "TestConsumer").Msg("failed to convert string to big int") + log.Error().Str("Player", "TestConsumer").Str("AggregateTime", result.AggregateTime).Msg("failed to convert string to big int")
60-64: Improve error logging for transaction creation.The error log message can be more descriptive to aid in debugging.
- log.Error().Err(err).Msg("MakeDirect") + log.Error().Err(err).Str("Player", "TestConsumer").Msg("failed to create transaction")
73-77: Improve error logging for transaction submission.The error log message can be more descriptive to aid in debugging.
- log.Error().Err(err).Msg("SubmitRawTx") + log.Error().Err(err).Str("Player", "TestConsumer").Msg("failed to submit transaction").github/workflows/dal.test.yaml (1)
61-67: Improve error logging for vet step.Consider adding more context to the error logs for the vet step.
- go vet ./... + go vet ./... || { echo 'Go vet failed'; exit 1; } - go vet -vettool=$(which shadow) ./... + go vet -vettool=$(which shadow) ./... || { echo 'Go vet with shadow failed'; exit 1; }node/pkg/dal/collector/collector.go (2)
57-58: Improve error logging for chain reader initialization.The error log message can be more descriptive to aid in debugging.
- return nil, err + return nil, fmt.Errorf("failed to create chain reader: %w", err)
62-63: Improve error logging for initial whitelist retrieval.The error log message can be more descriptive to aid in debugging.
- return nil, err + return nil, fmt.Errorf("failed to retrieve initial whitelist: %w", err)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
node/taskfiles/taskfile.local.ymlis excluded by!**/*.yml
Files selected for processing (7)
- .github/workflows/dal.test.yaml (1 hunks)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/common/types.go (1 hunks)
- node/pkg/dal/tests/api_test.go (1 hunks)
- node/pkg/dal/tests/collector_test.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
- node/script/test_dal_consumer/main.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- node/pkg/dal/common/types.go
- node/pkg/dal/tests/api_test.go
- node/pkg/dal/tests/collector_test.go
- node/pkg/dal/tests/main_test.go
Additional comments not posted (3)
node/pkg/dal/collector/collector.go (3)
99-103: LGTM!The
Stopfunction is well-structured and stops the collector gracefully.
131-139: LGTM!The
processIncomingDatafunction is well-structured and processes incoming data efficiently.
108-112: Add error handling or logging for goroutines.Consider adding error handling or logging for the goroutines to ensure any issues are captured.
- go func(id int32) { - if err := c.receiveEach(ctx, id); err != nil { - log.Error().Err(err).Str("Player", "DalCollector").Msg("Error in receiveEach goroutine") - } - }(id) + go func(id int32) { + if err := c.receiveEach(ctx, id); err != nil { + log.Error().Err(err).Str("Player", "DalCollector").Msg("Error in receiveEach goroutine") + } + }(id)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/dal.test.yaml (1 hunks)
- .github/workflows/node.test.yaml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/node.test.yaml
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/dal.test.yaml
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
go.work.sumis excluded by!**/*.sum,!**/*.sumnode/go.modis excluded by!**/*.modnode/go.sumis excluded by!**/*.sum,!**/*.sumnode/taskfiles/taskfile.local.ymlis excluded by!**/*.yml
Files selected for processing (15)
- .github/workflows/dal.test.yaml (1 hunks)
- node/.env.example (1 hunks)
- node/cmd/dal/main.go (1 hunks)
- node/pkg/dal/api/controller.go (1 hunks)
- node/pkg/dal/api/route.go (1 hunks)
- node/pkg/dal/api/types.go (1 hunks)
- node/pkg/dal/app.go (1 hunks)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/collector/utils.go (1 hunks)
- node/pkg/dal/common/types.go (1 hunks)
- node/pkg/dal/tests/api_test.go (1 hunks)
- node/pkg/dal/tests/collector_test.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
- node/pkg/dal/utils/utils.go (1 hunks)
- node/script/test_dal_consumer/main.go (1 hunks)
Files skipped from review as they are similar to previous changes (14)
- .github/workflows/dal.test.yaml
- node/.env.example
- node/cmd/dal/main.go
- node/pkg/dal/api/controller.go
- node/pkg/dal/api/route.go
- node/pkg/dal/api/types.go
- node/pkg/dal/app.go
- node/pkg/dal/collector/utils.go
- node/pkg/dal/common/types.go
- node/pkg/dal/tests/api_test.go
- node/pkg/dal/tests/collector_test.go
- node/pkg/dal/tests/main_test.go
- node/pkg/dal/utils/utils.go
- node/script/test_dal_consumer/main.go
Additional comments not posted (1)
node/pkg/dal/collector/collector.go (1)
106-112: Add error handling or logging for goroutines.Consider adding error handling or logging for the goroutines to ensure any issues are captured.
- go c.receiveEach(ctx, id) + go func(id int32) { + if err := c.receiveEach(ctx, id); err != nil { + log.Error().Err(err).Str("Player", "DalCollector").Msg("Error in receiveEach goroutine") + } + }(id)Likely invalid or redundant comment.
nick-bisonai
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
go.work.sumis excluded by!**/*.sum,!**/*.sumnode/go.modis excluded by!**/*.modnode/go.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (2)
- .github/workflows/deployment.yaml (1 hunks)
- dockerfiles/orakl-dal.Dockerfile (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/deployment.yaml
- dockerfiles/orakl-dal.Dockerfile
node/pkg/dal/collector/collector.go
Outdated
| FeedHashes map[int32][]byte | ||
| CachedWhitelist []klaytncommon.Address | ||
|
|
||
| Ctx context.Context |
There was a problem hiding this comment.
is there a specific reason we are storing the context in a struct? Apparently it's not a good practice to store context in a struct, rather it should be passed to functions that require it, unless there it's necessary: https://go.dev/blog/context-and-structs
There was a problem hiding this comment.
I kept ctx to track its running state,
ctx here is the one which is generated from param passed through Start func below.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- .github/workflows/dal.test.yaml (1 hunks)
- node/.env.example (1 hunks)
- node/pkg/admin/tests/test_helper.go (2 hunks)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/tests/api_test.go (1 hunks)
- node/pkg/dal/utils/utils.go (1 hunks)
- node/pkg/wss/utils.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/dal.test.yaml
- node/pkg/dal/collector/collector.go
- node/pkg/dal/tests/api_test.go
- node/pkg/dal/utils/utils.go
Additional comments not posted (10)
node/.env.example (7)
55-58: LGTM!The addition of the
DAL_API_PORTvariable is clear and well-commented.
60-61: LGTM!The addition of the
KAIA_WEBSOCKET_URLvariable is clear and well-commented.
63-64: LGTM!The addition of the
SUBMISSION_PROXY_CONTRACTvariable is clear and well-commented.
66-67: LGTM!The addition of the
API_KEYvariable is clear and well-commented.
69-70: LGTM!The addition of the
DATABASE_URLvariable is clear and well-commented.
71-71: LGTM!The addition of the
REDIS_HOSTvariable is clear and well-commented.
72-72: LGTM!The addition of the
REDIS_PORTvariable is clear and well-commented.node/pkg/admin/tests/test_helper.go (1)
59-65: LGTM!The code correctly sets the
Content-TypeandX-API-Keyheaders for HTTP requests.node/pkg/wss/utils.go (2)
25-35: LGTM!The code correctly adds support for request headers in the
ConnectionConfigstruct and provides a function to set these headers.Also applies to: 79-83
101-105: LGTM!The code correctly sets the request headers in the WebSocket connection options.
Also applies to: 137-143
wip feat: encapsulate wip wip feat: rest api
fix: linter err and vet err fix feat: rename and separate types fix: change function order, remove err return fix: update based on feedbacks docs: update .env.example
* fix: module update and add dockerfile * feat: add dal into application options (#1723)
* feat: implement basic api key * fix: init header * test: trigger dal test * fix: add log based on coderabbit feedback * test: start subscription before publish from test
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
node/go.modis excluded by!**/*.modnode/go.sumis excluded by!**/*.sum,!**/*.sumnode/taskfiles/taskfile.local.ymlis excluded by!**/*.yml
Files selected for processing (19)
- .github/workflows/dal.test.yaml (1 hunks)
- .github/workflows/deployment.yaml (1 hunks)
- dockerfiles/orakl-dal.Dockerfile (1 hunks)
- node/.env.example (1 hunks)
- node/cmd/dal/main.go (1 hunks)
- node/pkg/admin/tests/test_helper.go (2 hunks)
- node/pkg/dal/api/controller.go (1 hunks)
- node/pkg/dal/api/route.go (1 hunks)
- node/pkg/dal/api/types.go (1 hunks)
- node/pkg/dal/app.go (1 hunks)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/collector/utils.go (1 hunks)
- node/pkg/dal/common/types.go (1 hunks)
- node/pkg/dal/tests/api_test.go (1 hunks)
- node/pkg/dal/tests/collector_test.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
- node/pkg/dal/utils/utils.go (1 hunks)
- node/pkg/wss/utils.go (4 hunks)
- node/script/test_dal_consumer/main.go (1 hunks)
Files skipped from review due to trivial changes (1)
- node/pkg/wss/utils.go
Files skipped from review as they are similar to previous changes (17)
- .github/workflows/dal.test.yaml
- .github/workflows/deployment.yaml
- dockerfiles/orakl-dal.Dockerfile
- node/.env.example
- node/pkg/admin/tests/test_helper.go
- node/pkg/dal/api/controller.go
- node/pkg/dal/api/route.go
- node/pkg/dal/api/types.go
- node/pkg/dal/app.go
- node/pkg/dal/collector/collector.go
- node/pkg/dal/collector/utils.go
- node/pkg/dal/common/types.go
- node/pkg/dal/tests/api_test.go
- node/pkg/dal/tests/collector_test.go
- node/pkg/dal/tests/main_test.go
- node/pkg/dal/utils/utils.go
- node/script/test_dal_consumer/main.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
node/cmd/dal/main.go (1)
24-27: Enhance error logging.The error handling could be improved by providing more context in the log message.
- log.Fatal().Err(err).Msg("Failed to start DAL") + log.Fatal().Err(err).Msg("Failed to start DAL in main function")node/pkg/dal/tests/main_test.go (1)
1-2: Remove unnecessary nolint directive.The
//nolint:alldirective suppresses all linting errors, which is generally not recommended.- //nolint:all
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- node/cmd/dal/main.go (1 hunks)
- node/pkg/dal/tests/main_test.go (1 hunks)
Additional comments not posted (10)
node/cmd/dal/main.go (3)
1-11: Ensure proper package and import usage.The package and imports are necessary and correctly included.
13-15: Ensure proper context cancellation.The context is created and properly canceled using
defer cancel().
17-22: Handle OS interrupts and cancel context.The code correctly handles OS interrupts to cancel the context.
node/pkg/dal/tests/main_test.go (7)
3-22: Ensure proper package and import usage.The package and imports are necessary and correctly included.
24-29: Ensure proper struct definition.The
TestItemsstruct is defined correctly to hold test-related items.
31-33: Improve error handling intestPublishData.The function should handle errors properly to avoid silent failures.
if err := db.Publish(ctx, keys.SubmissionDataStreamKey(submissionData.GlobalAggregate.ConfigID), submissionData); err != nil { log.Error().Err(err).Msg("failed to publish data in testPublishData") }
35-64: Improve error handling and logging ingenerateSampleSubmissionData.The function should handle errors properly and log them for better debugging.
if err != nil { log.Error().Err(err).Msg("failed to create sign helper in generateSampleSubmissionData") return nil, err } if err != nil { log.Error().Err(err).Msg("failed to make global aggregate proof in generateSampleSubmissionData") return nil, err } return &aggregator.SubmissionData{ GlobalAggregate: sampleGlobalAggregate, Proof: proof, }, nil
66-101: Improve setup function with detailed error logging.The
setupfunction initializes test items and should log errors with more context for better debugging.if err != nil { log.Error().Err(err).Msg("error inserting config in setup") return nil, nil, err } if err != nil { log.Error().Err(err).Msg("error setting up Fiber app in setup") return nil, nil, err }
103-122: Ensure proper resource cleanup in test functions.The
cleanupfunction should handle errors properly and ensure all resources are cleaned up to avoid resource leaks.if err != nil { log.Error().Err(err).Msg("error deleting config in cleanup") return err } if err != nil { log.Error().Err(err).Msg("error shutting down app in cleanup") return err }
124-134: Ensure proper setup and teardown inTestMain.The
TestMainfunction should ensure proper setup and teardown of resources to avoid resource leaks.// setup code := m.Run() // teardown db.ClosePool() db.CloseRedis() os.Exit(code)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- node/pkg/dal/collector/collector.go (1 hunks)
- node/pkg/dal/tests/api_test.go (1 hunks)
- node/pkg/dal/tests/collector_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- node/pkg/dal/collector/collector.go
- node/pkg/dal/tests/api_test.go
- node/pkg/dal/tests/collector_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/dal.test.yaml (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/dal.test.yaml
Description
Data Availability Layer Beta
default port 8090
Rest API
/api/v1/dal/latest-data-feeds/all: get data for onchain submission for all possible pairs/api/v1/dal/latest-data-feeds/btc-usdt: get data for onchain submission for certain pairWebsockert API
/api/v1/dal/wsResponse Example
Type of change
Please delete options that are not relevant.
Checklist before requesting a review
Deployment