[TODO] Blockers @okdas #674
Conversation
WalkthroughThe changes span multiple files and focus on enhancing code quality, error handling, and linter configurations. Notable updates include enabling specific linters in 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 (
|
|
Skipping e2e test as no go code changes were made. I did verify the LocalNet set up though, as the Tiltfile was changed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
Tiltfile (1)
153-161: Issue Found: Missing Validator KeysThe paths specified in the Tiltfile for
node_key.jsonandpriv_validator_key.jsonare incorrect, as these files do not exist anywhere in the repository. Please ensure that these files are added to the repository or update the paths accordingly.
localnet/poktrolld/config/node_key.json- missinglocalnet/poktrolld/config/priv_validator_key.json- missingAnalysis chain
LGTM! Ensure the paths for validator keys are correct.
Uncommenting this section and providing the paths enables the Helm chart to consume validator keys. Verify that the paths to
node_key.jsonandpriv_validator_key.jsonare correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the paths for validator keys in the Tiltfile. # Test: Ensure the files exist at the specified paths. # Expect: Both files should exist. test -f "localnet/poktrolld/config/node_key.json" && echo "node_key.json exists" || echo "node_key.json missing" test -f "localnet/poktrolld/config/priv_validator_key.json" && echo "priv_validator_key.json exists" || echo "priv_validator_key.json missing"Length of output: 235
Script:
#!/bin/bash # Description: Search for the files node_key.json and priv_validator_key.json within the entire repository # Look for node_key.json echo "Searching for node_key.json..." fd --type f --name node_key.json # Look for priv_validator_key.json echo "Searching for priv_validator_key.json..." fd --type f --name priv_validator_key.jsonLength of output: 743
Script:
#!/bin/bash # Description: Search for the files node_key.json and priv_validator_key.json within the entire repository # Look for node_key.json echo "Searching for node_key.json..." fd node_key.json # Look for priv_validator_key.json echo "Searching for priv_validator_key.json..." fd priv_validator_key.jsonLength of output: 209
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- .golangci.yml (2 hunks)
- Makefile (1 hunks)
- Tiltfile (1 hunks)
- x/shared/helpers/service_configs.go (1 hunks)
Files skipped from review due to trivial changes (2)
- Makefile
- x/shared/helpers/service_configs.go
Additional comments not posted (4)
.golangci.yml (4)
3-4: LGTM! Enabling theshadowcheck is a good practice.The
shadowcheck helps catch variable shadowing issues early.
9-15: LGTM! Enabling multiple linters enhances code quality.The enabled linters (
errcheck,gosimple,goimports,govet,ineffassign,staticcheck,unused) will help catch a wide range of issues.
47-48: LGTM! Excluding test files fromerrcheckcan reduce false positives.Test files are often written in a way that might trigger unnecessary
errcheckwarnings.
19-20: LGTM! Comment cleanup improves code readability.Removing unnecessary comments helps maintain a cleaner codebase.
Also applies to: 29-30
There was a problem hiding this comment.
Nice one @okdas - thanks for picking this back up! 🔥
What was your thinking on the order of operations to getting CI passing on main?
EDIT: I opened a PR with the linter fixes. I figure once it has approval, we can merge it into this branch prior to merging this with main. Wdyt?
| // TODO_MAINNET(@okdas)/TODO_DISCUSS: Either add validation for `endpoint.Configs` (can be a part of | ||
| // `parseEndpointConfigs`), or change the config structure to be more clear about what is expected here | ||
| // as currently, this is just a map[string]string, when values can be other types. |
There was a problem hiding this comment.
@bryanchriswhite had a chat with @red-0ne in Discord so I'll adjust the comment accordingly.
Co-authored-by: Dmitry K. <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 10
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (57)
- app/app.go (2 hunks)
- app/export.go (8 hunks)
- app/ibc.go (1 hunks)
- cmd/poktrolld/cmd/commands.go (1 hunks)
- cmd/poktrolld/cmd/root.go (6 hunks)
- docs/docs.go (1 hunks)
- go.mod (2 hunks)
- pkg/appgateserver/sdkadapter/sdk.go (2 hunks)
- pkg/appgateserver/server.go (2 hunks)
- pkg/client/events/query_client.go (1 hunks)
- pkg/client/events/replay_client.go (2 hunks)
- pkg/client/events/replay_client_integration_test.go (1 hunks)
- pkg/client/tx/client.go (2 hunks)
- pkg/crypto/protocol/errors.go (1 hunks)
- pkg/crypto/rand/float_test.go (2 hunks)
- pkg/crypto/rings/cache.go (2 hunks)
- pkg/crypto/rings/client.go (2 hunks)
- pkg/deps/config/suppliers.go (4 hunks)
- pkg/observable/channel/observable_test.go (2 hunks)
- pkg/observable/channel/observer_manager.go (6 hunks)
- pkg/observable/channel/replay_test.go (2 hunks)
- pkg/polylog/context.go (2 hunks)
- pkg/polylog/context_test.go (1 hunks)
- pkg/polylog/polyzero/logger.go (1 hunks)
- pkg/polylog/polyzero/logger_test.go (1 hunks)
- pkg/relayer/config/pocket_node_config_hydrator.go (1 hunks)
- pkg/relayer/miner/gen/template.go (1 hunks)
- pkg/relayer/proxy/proxy_test.go (8 hunks)
- pkg/relayer/proxy/relay_verifier.go (1 hunks)
- pkg/relayer/proxy/server_builder.go (1 hunks)
- pkg/relayer/proxy/synchronous.go (2 hunks)
- pkg/relayer/relayminer.go (1 hunks)
- testutil/integration/app.go (1 hunks)
- testutil/keeper/shared.go (1 hunks)
- testutil/keeper/tokenomics.go (1 hunks)
- testutil/network/network.go (1 hunks)
- testutil/proof/fixture_generators.go (1 hunks)
- testutil/testclient/testeventsquery/client.go (1 hunks)
- testutil/testpolylog/event.go (2 hunks)
- testutil/testproxy/relayerproxy.go (3 hunks)
- x/application/keeper/msg_server_delegate_to_gateway.go (1 hunks)
- x/application/keeper/msg_server_delegate_to_gateway_test.go (3 hunks)
- x/application/module/simulation.go (1 hunks)
- x/gateway/module/simulation.go (1 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (10 hunks)
- x/proof/module/helpers_test.go (4 hunks)
- x/proof/module/query_claim.go (1 hunks)
- x/proof/module/query_claim_test.go (9 hunks)
- x/proof/module/query_proof.go (2 hunks)
- x/proof/module/simulation.go (1 hunks)
- x/proof/types/params.go (1 hunks)
- x/proof/types/query_validation.go (2 hunks)
- x/service/module/simulation.go (1 hunks)
- x/session/module/query_get_session.go (1 hunks)
- x/session/module/simulation.go (1 hunks)
- x/shared/module/genesis.go (2 hunks)
- x/shared/module/simulation.go (1 hunks)
Files not processed due to max files limit (8)
- x/shared/session_test.go
- x/supplier/module/simulation.go
- x/tokenomics/keeper/keeper_settle_pending_claims_test.go
- x/tokenomics/keeper/proof_requirement_test.go
- x/tokenomics/keeper/settle_pending_claims.go
- x/tokenomics/keeper/settle_session_accounting.go
- x/tokenomics/keeper/update_relay_mining_difficulty.go
- x/tokenomics/module/simulation.go
Files skipped from review due to trivial changes (20)
- app/ibc.go
- cmd/poktrolld/cmd/commands.go
- docs/docs.go
- pkg/client/events/query_client.go
- pkg/crypto/protocol/errors.go
- pkg/crypto/rings/cache.go
- pkg/observable/channel/observable_test.go
- pkg/polylog/context_test.go
- pkg/polylog/polyzero/logger_test.go
- pkg/relayer/config/pocket_node_config_hydrator.go
- pkg/relayer/miner/gen/template.go
- pkg/relayer/proxy/relay_verifier.go
- pkg/relayer/proxy/server_builder.go
- testutil/network/network.go
- testutil/proof/fixture_generators.go
- testutil/testpolylog/event.go
- x/application/keeper/msg_server_delegate_to_gateway.go
- x/proof/module/query_proof.go
- x/proof/types/params.go
- x/proof/types/query_validation.go
Additional comments not posted (86)
testutil/keeper/shared.go (1)
46-47: Improve error handling in SharedKeeper.The error handling added to
SharedKeeperis a good improvement. However, the error message formatting can be refined by removing the%vplaceholder as%walready includes the error message.- err := k.SetParams(ctx, types.DefaultParams()) + err := k.SetParams(ctx, types.DefaultParams())Likely invalid or redundant comment.
pkg/polylog/context.go (1)
9-15: LGTM! The addition ofPolylogCtxKeyimproves clarity.The new constant
PolylogCtxKeydisambiguates the context key used for storing the logger, which is a good improvement.x/session/module/simulation.go (1)
46-47: LGTM! The update toProposalContentsaligns with the new simulation framework.The function now returns
[]simtypes.WeightedProposalMsginstead of[]simtypes.WeightedProposalContent, which is consistent with the updated framework.x/service/module/simulation.go (1)
50-51: LGTM! The update toProposalContentsaligns with the new simulation framework.The function now returns
[]simtypes.WeightedProposalMsginstead of[]simtypes.WeightedProposalContent, which is consistent with the updated framework.x/shared/module/simulation.go (1)
50-51: LGTM! The update toProposalContentsaligns with the new simulation framework.The function now returns
[]simtypes.WeightedProposalMsginstead of[]simtypes.WeightedProposalContent, which is consistent with the updated framework.pkg/polylog/polyzero/logger.go (1)
100-102: LGTM!The change correctly adds the logger to the context, enhancing functionality.
pkg/appgateserver/sdkadapter/sdk.go (6)
36-38: Good error handling improvement.The change correctly checks for errors during session client initialization.
41-43: Good error handling improvement.The change correctly checks for errors during account client initialization.
46-48: Good error handling improvement.The change correctly checks for errors during application client initialization.
52-53: Good error handling improvement.The change correctly checks for errors during dependency injection.
56-58: Good error handling improvement.The change correctly checks for errors during signer initialization.
96-98: Good error handling improvement.The change correctly checks for errors during relay request signing.
x/gateway/module/simulation.go (1)
54-55: Good alignment with the updated simulation framework.The change correctly updates the return type of the
ProposalContentsfunction.testutil/testclient/testeventsquery/client.go (1)
48-48: LGTM!The changes to the
NewOneTimeEventsQueryfunction properly manage the mutex and initialize the observable.x/proof/module/simulation.go (1)
58-58: LGTM!The
ProposalContentsfunction now correctly returns[]simtypes.WeightedProposalMsgto align with the new simulation framework.pkg/relayer/relayminer.go (2)
124-126: LGTM!The
ServePprofmethod correctly handles errors and manages the context for shutting down the server.
132-132: LGTM!The context management for shutting down the server is correctly implemented.
x/proof/module/query_claim.go (4)
34-37: LGTM!The error handling and request validation in the
CmdListClaimsfunction are correctly implemented.
43-46: LGTM!The error handling and request validation in the
CmdListClaimsfunction are correctly implemented.
50-52: LGTM!The error handling and request validation in the
CmdListClaimsfunction are correctly implemented.
56-58: LGTM!The error handling and request validation in the
CmdListClaimsfunction are correctly implemented.cmd/poktrolld/cmd/root.go (4)
4-5: Imports look good!The added imports for
errors,log, andcosmoslogare appropriate and necessary for the functionality.Also applies to: 13-13
48-48: Improved error handling and initialization.The changes in error handling and initialization of the client context enhance the robustness and clarity of the code.
Also applies to: 67-67, 73-73, 96-96, 100-100, 120-125
144-161: New function for overwriting flag defaults looks good!The function
overwriteFlagDefaultsis well-structured and effectively handles setting default flag values.
Line range hint
163-184:
Client context initialization looks good!The function
ProvideClientContextis well-structured and effectively initializes the client context.x/application/module/simulation.go (1)
62-62: Updated return type forProposalContentslooks good!The change to return
[]simtypes.WeightedProposalMsgaligns with the updated simulation framework and improves compatibility.x/proof/module/helpers_test.go (1)
40-40: Improved error handling and client context initialization.The changes in error handling and client context initialization enhance the robustness and clarity of the code.
Also applies to: 118-119, 134-134
app/export.go (6)
97-99: Error handling improvement.The error handling for
sdk.ValAddressFromBech32is correctly implemented and improves code clarity.
119-121: Error handling improvement.The error handling for
app.StakingKeeper.ValidatorAddressCodec().StringToBytesis correctly implemented and improves code clarity.
124-126: Error handling improvement.The error handling for
app.DistrKeeper.GetValidatorOutstandingRewardsCoinsis correctly implemented and improves code clarity.
128-130: Error handling improvement.The error handling for
app.DistrKeeper.FeePool.Getis correctly implemented and improves code clarity.
133-133: Error handling improvement.The error handling for
app.DistrKeeper.FeePool.Setis correctly implemented and improves code clarity.
137-137: Error handling improvement.The error handling for
app.DistrKeeper.Hooks().AfterValidatorCreatedis correctly implemented and improves code clarity.x/proof/module/query_claim_test.go (4)
27-27: Setup improvement.The setup process for the network with claim objects is improved.
100-100: Error handling improvement.The error handling for the CLI command execution is correctly implemented and improves the test case.
142-142: Setup improvement.The setup process for the network with claim objects is improved.
164-164: Error handling improvement.The error handling for the CLI command execution is correctly implemented and improves the test case.
pkg/observable/channel/replay_test.go (2)
142-142: Assertion improvement.The assertion for equality of expected and actual values is correctly implemented and improves the test case.
223-229: Timeout handling improvement.The timeout handling for the context and the call to the
Lastmethod is correctly implemented and improves the test case.x/application/keeper/msg_server_delegate_to_gateway_test.go (2)
231-232: Check improvement.The check for finding the application after staking is correctly implemented and improves the test case.
251-252: Check improvement.The check for finding the application after delegating to gateways is correctly implemented and improves the test case.
testutil/keeper/tokenomics.go (1)
251-254: LGTM!The changes ensure that coins are minted for the supplier and application modules with proper error handling.
pkg/appgateserver/server.go (2)
139-139: LGTM!The changes ensure that the server is properly shut down when the context is done.
303-305: LGTM!The changes ensure that the pprof server is started and properly shut down when the context is done.
Also applies to: 311-311
pkg/client/events/replay_client.go (1)
243-243: LGTM!The changes ensure that errors are properly handled when getting event bytes.
pkg/crypto/rings/client.go (1)
355-355: LGTM!The changes ensure that the ring points map contains the public keys in the ring signature.
go.mod (1)
82-83: Addition ofgolang.org/x/textdependency looks good.The addition of
golang.org/x/text v0.14.0aligns with the PR objectives and improves code health.testutil/testproxy/relayerproxy.go (3)
8-8: Import oferrorspackage looks good.The addition of the
errorspackage import supports enhanced error handling.
155-162: Starting HTTP server in a goroutine looks good.The added goroutine improves the server's robustness by handling errors during startup.
165-166: Graceful server shutdown logic looks good.The added goroutine ensures graceful shutdown of the server when the context is done.
pkg/deps/config/suppliers.go (4)
138-138: Settinggrpc-addrflag looks good.The change ensures the correct gRPC address is used for the client context.
199-199: Settingnodeflag looks good.The change ensures the correct node address is used for the client context.
208-208: Settinggrpc-addrflag looks good.The change ensures the correct gRPC address is used for the client context.
217-217: Restoringchain-idflag looks good.The change ensures the correct chain ID is restored after setting the client context.
pkg/relayer/proxy/synchronous.go (3)
87-87: Graceful server shutdown logic looks good.The change ensures graceful shutdown of the server when the context is done.
121-121: Relay request validation looks good.The change ensures the relay request is validated before processing.
Line range hint
329-329: Relay response validation looks good.The change ensures the relay response is validated before processing.
app/app.go (2)
193-193: LGTM! Variable renamed for clarity.The variable
appConfigis renamed todepsfor better clarity.
249-249: LGTM! Consistent variable renaming.The variable
appConfigis renamed todepsin thedepinject.Injectcall, maintaining consistency.pkg/client/tx/client.go (1)
163-163: LGTM! Improved error handling.Error handling is added to the
validateConfigAndSetDefaultsfunction call, enhancing robustness.testutil/integration/app.go (2)
555-556: LGTM! Improved error handling.Error handling is added to the
MintCoinscall for thesuppliertypes.ModuleName, enhancing robustness.
557-558: LGTM! Improved error handling.Error handling is added to the
MintCoinscall for theapptypes.ModuleName, enhancing robustness.pkg/relayer/proxy/proxy_test.go (6)
137-138: LGTM!The use of
context.WithCancelis appropriate for managing the context lifecycle in tests.
172-173: LGTM!The use of
context.WithCancelis appropriate for managing the context lifecycle in tests.
189-190: LGTM!The use of
context.WithCancelis appropriate for managing the context lifecycle in tests.
203-204: LGTM!The use of
context.WithCancelis appropriate for managing the context lifecycle in tests.
219-220: LGTM!The use of
context.WithCancelis appropriate for managing the context lifecycle in tests.
256-257: LGTM!The use of
context.WithCancelis appropriate for managing the context lifecycle in tests.x/proof/keeper/msg_server_submit_proof_test.go (7)
694-695: LGTM! Error handling for session tree flushing.The added error handling ensures that any issues during the flushing of the session tree are caught and managed.
748-749: LGTM! Error handling for session tree flushing.The added error handling ensures that any issues during the flushing of the session tree are caught and managed.
803-804: LGTM! Error handling for session tree flushing.The added error handling ensures that any issues during the flushing of the session tree are caught and managed.
851-852: LGTM! Error handling for relay marshaling.The added error handling ensures that any issues during the marshaling of the relay are caught and managed.
917-918: LGTM! Error handling for relay marshaling.The added error handling ensures that any issues during the marshaling of the relay are caught and managed.
929-930: LGTM! Error handling for session tree flushing.The added error handling ensures that any issues during the flushing of the session tree are caught and managed.
985-986: LGTM! Error handling for session tree flushing.The added error handling ensures that any issues during the flushing of the session tree are caught and managed.
pkg/client/events/replay_client_integration_test.go (11)
Line range hint
1-13:
Import statements look good.The necessary packages for testing and the required dependencies are imported.
Line range hint
15-18:
Ensure interface implementation.The
messageEventinterface andtEventstruct are correctly defined and implemented.
Line range hint
20-30:
Decoder function looks good.The
newDecodeEventMessageFnfunction correctly decodes JSON-encoded event messages.
32-34: Helper function for creating event messages looks good.The
newMessageEventBzfunction correctly formats event messages in JSON.
Line range hint
36-39:
Test function setup looks good.The initial setup for the test function
TestReplayClient_Remappingis clear and well-structured.
Line range hint
41-44:
Mock connection and dialer setup looks good.The mock connection and dialer are correctly set up for testing.
Line range hint
46-66:
Mock connection behavior is well-defined.The behavior of the mock connection, including event receiving and connection closing, is correctly defined.
Line range hint
68-71:
Query client dependency setup looks good.The events query client dependency is correctly set up.
Line range hint
73-81:
Replay client creation looks good.The replay client is correctly created and initialized.
Line range hint
83-92:
Event handling looks good.The event handling logic correctly processes received events and updates the event counter.
Line range hint
94-104:
Error handling and verification look good.The error handling and verification logic correctly ensure that the expected number of events are received.
| if err := k.SetParams(ctx, genState.Params); err != nil { | ||
| panic(fmt.Errorf("unable to set params: %v: %w", genState.Params, err)) | ||
| } |
There was a problem hiding this comment.
Improve error handling in InitGenesis.
The error handling added to InitGenesis is a good improvement. However, the error message formatting can be refined by removing the %v placeholder as %w already includes the error message.
- panic(fmt.Errorf("unable to set params: %v: %w", genState.Params, err))
+ panic(fmt.Errorf("unable to set params: %w", err))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := k.SetParams(ctx, genState.Params); err != nil { | |
| panic(fmt.Errorf("unable to set params: %v: %w", genState.Params, err)) | |
| } | |
| if err := k.SetParams(ctx, genState.Params); err != nil { | |
| panic(fmt.Errorf("unable to set params: %w", err)) | |
| } |
| go func() { | ||
| wg.Wait() | ||
| close(errCh) | ||
| }() | ||
|
|
||
| err := <-errCh | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Ensure proper error handling.
The added error handling for the wait group is a good improvement. However, using select to handle the error channel can ensure that the test does not hang if there are no errors.
go func() {
wg.Wait()
close(errCh)
}()
select {
case err := <-errCh:
require.NoError(t, err)
case <-time.After(time.Second * 10):
t.Fatal("test timed out")
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func() { | |
| wg.Wait() | |
| close(errCh) | |
| }() | |
| err := <-errCh | |
| require.NoError(t, err) | |
| go func() { | |
| wg.Wait() | |
| close(errCh) | |
| }() | |
| select { | |
| case err := <-errCh: | |
| require.NoError(t, err) | |
| case <-time.After(time.Second * 10): | |
| t.Fatal("test timed out") | |
| } |
| errCh := make(chan error, 1) | ||
| for idx := int64(0); idx < sampleSize; idx++ { | ||
| wg.Add(1) | ||
| go func() { | ||
| go func(idx int64) { | ||
| idxBz := make([]byte, binary.MaxVarintLen64) | ||
| binary.PutVarint(idxBz, idx) | ||
| randFloat, err := poktrand.SeededFloat32(idxBz) | ||
| require.NoError(t, err) | ||
|
|
||
| if randFloat < 0 || randFloat > 1 { | ||
| t.Fatalf("secureRandFloat64() returned out of bounds value: %f", randFloat) | ||
| errCh <- fmt.Errorf("secureRandFloat64() returned out of bounds value: %f", randFloat) | ||
| wg.Done() | ||
| return |
There was a problem hiding this comment.
Ensure proper synchronization and error handling.
The added error handling and synchronization are good improvements. However, using defer wg.Done() at the beginning of the goroutine can simplify the code and ensure wg.Done() is always called.
- go func(idx int64) {
+ go func(idx int64) {
+ defer wg.Done()
idxBz := make([]byte, binary.MaxVarintLen64)
binary.PutVarint(idxBz, idx)
randFloat, err := poktrand.SeededFloat32(idxBz)
require.NoError(t, err)
if randFloat < 0 || randFloat > 1 {
errCh <- fmt.Errorf("secureRandFloat64() returned out of bounds value: %f", randFloat)
- wg.Done()
return
}
if randFloat <= probability {
numTrueSamples.Add(1)
}
- wg.Done()
}(idx)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| errCh := make(chan error, 1) | |
| for idx := int64(0); idx < sampleSize; idx++ { | |
| wg.Add(1) | |
| go func() { | |
| go func(idx int64) { | |
| idxBz := make([]byte, binary.MaxVarintLen64) | |
| binary.PutVarint(idxBz, idx) | |
| randFloat, err := poktrand.SeededFloat32(idxBz) | |
| require.NoError(t, err) | |
| if randFloat < 0 || randFloat > 1 { | |
| t.Fatalf("secureRandFloat64() returned out of bounds value: %f", randFloat) | |
| errCh <- fmt.Errorf("secureRandFloat64() returned out of bounds value: %f", randFloat) | |
| wg.Done() | |
| return | |
| errCh := make(chan error, 1) | |
| for idx := int64(0); idx < sampleSize; idx++ { | |
| wg.Add(1) | |
| go func(idx int64) { | |
| defer wg.Done() | |
| idxBz := make([]byte, binary.MaxVarintLen64) | |
| binary.PutVarint(idxBz, idx) | |
| randFloat, err := poktrand.SeededFloat32(idxBz) | |
| require.NoError(t, err) | |
| if randFloat < 0 || randFloat > 1 { | |
| errCh <- fmt.Errorf("secureRandFloat64() returned out of bounds value: %f", randFloat) | |
| return | |
| } | |
| if randFloat <= probability { | |
| numTrueSamples.Add(1) | |
| } | |
| }(idx) |
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| appAddressString := args[0] | ||
| serviceIdString := args[1] | ||
| blockHeightString := "0" // 0 will default to latest height | ||
| if len(args) == 3 { | ||
| blockHeightString = args[2] | ||
| } | ||
|
|
||
| blockHeight, err := strconv.ParseInt(blockHeightString, 10, 64) | ||
| if err != nil { | ||
| return fmt.Errorf("couldn't convert block height to int: %s; (%v)", blockHeightString, err) | ||
| blockHeight, parseErr := strconv.ParseInt(blockHeightString, 10, 64) | ||
| if parseErr != nil { | ||
| return fmt.Errorf("couldn't convert block height to int: %s; (%v)", blockHeightString, parseErr) |
There was a problem hiding this comment.
Ensure proper error handling in CmdGetSession.
The added error handling for parsing the block height is a good improvement. However, the error message formatting can be refined by removing the %s placeholder as %v already includes the error message.
- return fmt.Errorf("couldn't convert block height to int: %s; (%v)", blockHeightString, parseErr)
+ return fmt.Errorf("couldn't convert block height to int: %w", parseErr)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RunE: func(cmd *cobra.Command, args []string) error { | |
| appAddressString := args[0] | |
| serviceIdString := args[1] | |
| blockHeightString := "0" // 0 will default to latest height | |
| if len(args) == 3 { | |
| blockHeightString = args[2] | |
| } | |
| blockHeight, err := strconv.ParseInt(blockHeightString, 10, 64) | |
| if err != nil { | |
| return fmt.Errorf("couldn't convert block height to int: %s; (%v)", blockHeightString, err) | |
| blockHeight, parseErr := strconv.ParseInt(blockHeightString, 10, 64) | |
| if parseErr != nil { | |
| return fmt.Errorf("couldn't convert block height to int: %s; (%v)", blockHeightString, parseErr) | |
| blockHeight, parseErr := strconv.ParseInt(blockHeightString, 10, 64) | |
| if parseErr != nil { | |
| return fmt.Errorf("couldn't convert block height to int: %w", parseErr) |
| clientCtx, ctxErr := client.GetClientQueryContext(cmd) | ||
| if ctxErr != nil { | ||
| return ctxErr |
There was a problem hiding this comment.
Ensure proper error handling in CmdGetSession.
The added error handling for getting the client context is a good improvement. However, the error message formatting can be refined by removing the %s placeholder as %v already includes the error message.
- return fmt.Errorf("couldn't get client context: %s; (%v)", clientCtx, ctxErr)
+ return fmt.Errorf("couldn't get client context: %w", ctxErr)Committable suggestion was skipped due to low confidence.
| @@ -52,7 +52,7 @@ func newObserverManager[V any]() *channelObserverManager[V] { | |||
| } | |||
| } | |||
|
|
|||
| func (com *channelObserverManager[V]) notifyAll(notification V) { | |||
| func (com *channelObserverManager[V]) notifyAll(notification V) { //nolint:unused // Used in the observable implementation. | |||
There was a problem hiding this comment.
TODO comment for potential blocking issue.
The TODO comment highlights a potential issue with synchronous notification, suggesting a future improvement.
Would you like me to open a GitHub issue to track this improvement?
| @@ -74,7 +74,7 @@ func (com *channelObserverManager[V]) notifyAll(notification V) { | |||
| // by the channelObservable implementation as well as embedders of observerManager | |||
| // (e.g. replayObservable). | |||
| // It panics if toAdd is not a channelObserver. | |||
| func (com *channelObserverManager[V]) add(toAdd observable.Observer[V]) { | |||
| func (com *channelObserverManager[V]) add(toAdd observable.Observer[V]) { //nolint:unused // Used in the observable implementation. | |||
There was a problem hiding this comment.
TODO comment for casting improvement.
The TODO comment suggests a potential improvement to avoid casting.
Would you like me to open a GitHub issue to track this improvement?
| @@ -86,7 +86,7 @@ func (com *channelObserverManager[V]) add(toAdd observable.Observer[V]) { | |||
| // It implements the respective member of observerManager and is used by | |||
| // the channelObservable implementation as well as embedders of observerManager | |||
| // (e.g. replayObservable). | |||
| func (com *channelObserverManager[V]) remove(toRemove observable.Observer[V]) { | |||
| func (com *channelObserverManager[V]) remove(toRemove observable.Observer[V]) { //nolint:unused // Used in the observable implementation. | |||
There was a problem hiding this comment.
TODO comment for casting improvement.
The TODO comment suggests a potential improvement to avoid casting.
Would you like me to open a GitHub issue to track this improvement?
| @@ -103,7 +103,7 @@ func (com *channelObserverManager[V]) remove(toRemove observable.Observer[V]) { | |||
| // It implements the respective member of observerManager and is used by | |||
| // the channelObservable implementation as well as embedders of observerManager | |||
| // (e.g. replayObservable). | |||
| func (com *channelObserverManager[V]) removeAll() { | |||
| func (com *channelObserverManager[V]) removeAll() { //nolint:unused // Used in the observable implementation. | |||
There was a problem hiding this comment.
TODO comment for unsubscribing improvement.
The TODO comment suggests a potential improvement to handle unsubscribing more effectively.
Would you like me to open a GitHub issue to track this improvement?
| func (com *channelObserverManager[V]) goUnsubscribeOnDone( //nolint:unused // Used in the observable implementation. | ||
| ctx context.Context, | ||
| observer observable.Observer[V], | ||
| ) { | ||
| ) { //nolint:unused // Used in the observable implementation. |
There was a problem hiding this comment.
TODO comment for blocking improvement.
The TODO comment suggests a potential improvement to handle blocking more effectively.
Would you like me to open a GitHub issue to track this improvement?
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,14 +1,16 @@ | |||
| package cmd | |||
|
|
|||
| import ( | |||
| "errors" | |||
| "log" | |||
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
"log"
|
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. You may need to run GCP workloads (requires changing the namespace to 674) |
github-actions
bot
added
devnet
push-image
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range, codebase verification and nitpick comments (2)
x/shared/session_test.go (2)
Line range hint
60-69: Improve synchronization and error handling in concurrent tests.The addition of the
require.Equalfwithin the goroutine improves the assertion clarity. However, ensure that any failure in the assertion properly propagates to the main test routine. Consider using a channel to capture assertion failures or other errors to handle them appropriately in the main test routine.
Line range hint
128-140: Consistent error handling and synchronization in deterministic tests.The use of
assert.Equalfinside a goroutine needs careful handling to ensure that test failures are properly managed. Consider using a more robust error propagation strategy, such as error channels, especially since this is a highly concurrent test environment.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- go.mod (2 hunks)
- pkg/observable/channel/replay_test.go (2 hunks)
- pkg/relayer/proxy/proxy_test.go (8 hunks)
- pkg/relayer/proxy/relay_verifier.go (1 hunks)
- pkg/relayer/proxy/server_builder.go (1 hunks)
- pkg/relayer/proxy/synchronous.go (2 hunks)
- testutil/integration/app.go (1 hunks)
- testutil/keeper/tokenomics.go (1 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (10 hunks)
- x/shared/session_test.go (6 hunks)
- x/tokenomics/keeper/keeper_settle_pending_claims_test.go (2 hunks)
- x/tokenomics/keeper/settle_pending_claims.go (1 hunks)
Files skipped from review due to trivial changes (2)
- pkg/relayer/proxy/relay_verifier.go
- x/tokenomics/keeper/keeper_settle_pending_claims_test.go
Files skipped from review as they are similar to previous changes (8)
- go.mod
- pkg/observable/channel/replay_test.go
- pkg/relayer/proxy/proxy_test.go
- pkg/relayer/proxy/server_builder.go
- pkg/relayer/proxy/synchronous.go
- testutil/integration/app.go
- testutil/keeper/tokenomics.go
- x/proof/keeper/msg_server_submit_proof_test.go
| @@ -37,7 +37,7 @@ func TestGetEarliestSupplierClaimCommitHeight_IsDeterministic(t *testing.T) { | |||
| supplierAddr := sample.AccAddress() | |||
| var claimWindowOpenBlockHash [32]byte | |||
|
|
|||
| _, err := rand.Read(claimWindowOpenBlockHash[:]) | |||
| _, err := rand.Read(claimWindowOpenBlockHash[:]) //nolint:staticcheck // We need a deterministic pseudo-random source. | |||
There was a problem hiding this comment.
Ensure proper error handling in goroutines.
It's good to see error handling added for the rand.Read function. However, the error handling inside a goroutine should ensure that the entire test is aborted if an error occurs, not just the goroutine. This can be achieved by passing a channel to capture errors or using a panic-recover mechanism.
| @@ -99,7 +99,7 @@ | |||
| queryHeight := rand.Int63() | |||
| supplierAddr := sample.AccAddress() | |||
| var proofWindowOpenBlockHash [32]byte | |||
| _, err := rand.Read(proofWindowOpenBlockHash[:]) | |||
| _, err := rand.Read(proofWindowOpenBlockHash[:]) //nolint:staticcheck // We need a deterministic pseudo-random source. | |||
There was a problem hiding this comment.
Consistent error handling in goroutines for deterministic tests.
Similar to the previous comment, ensure that the error from rand.Read is handled appropriately. Using assert.NoError here is less strict than require.NoError used previously. Consider using require.NoError for consistency and to ensure the test stops immediately on error.
| @@ -72,7 +72,7 @@ | |||
| return settledResult, expiredResult, err | |||
| } | |||
|
|
|||
| logger := k.logger.With( | |||
| logger = k.logger.With( | |||
There was a problem hiding this comment.
Sensitive Information Logging
The fields sessionId and SupplierAddress being logged in x/tokenomics/keeper/settle_pending_claims.go are sensitive. Logging these fields could expose session identifiers and addresses, potentially leading to security risks. Please review the necessity of logging these fields. If logging is essential, consider obfuscating or anonymizing them to protect sensitive data.
- File:
x/tokenomics/keeper/settle_pending_claims.go - Lines: 75-75
Analysis chain
Enhanced logging for better traceability.
Adding detailed logging information is beneficial for debugging and monitoring. Ensure that the logged data does not include sensitive information that could compromise security or privacy. Review the fields being logged to confirm they are appropriate for public logs.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that no sensitive information is logged.
# Test: Check the logger setup. Expect: No sensitive fields.
rg --type go "logger = k.logger.With" x/tokenomics/keeper/settle_pending_claims.go
Length of output: 110
Script:
#!/bin/bash
# Extract surrounding lines of the logger setup to review the fields being logged.
rg --type go -A 5 -B 5 "logger = k.logger.With" x/tokenomics/keeper/settle_pending_claims.go
Length of output: 478
Script:
#!/bin/bash
# Search for the definitions and usages of "sessionId" and "SupplierAddress" to understand their nature.
# Find the definition and usage of sessionId
rg --type go "sessionId"
# Find the definition and usage of SupplierAddress
rg --type go "SupplierAddress"
Length of output: 55945
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)
- x/gateway/keeper/msg_server_unstake_gateway.go (1 hunks)
- x/proof/keeper/msg_server_submit_proof_test.go (12 hunks)
- x/tokenomics/keeper/settle_pending_claims.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/proof/keeper/msg_server_submit_proof_test.go
- x/tokenomics/keeper/settle_pending_claims.go
Additional comments not posted (1)
x/gateway/keeper/msg_server_unstake_gateway.go (1)
61-65: Review of added error handling in event emission.The addition of error handling for the event emission process is a crucial improvement. This ensures that any failures during event emission are caught and handled appropriately, maintaining the robustness of the function.
However, consider logging more context-specific information or metrics that could help in debugging or monitoring the system more effectively.
|
@bryanchriswhite since you've reviewed this PR and also #684, I'm going to merge this. One note I want to add is I had to also add extra changes in this commit to make linter happy. Please let me know if any of these do not make sense. |
…ation-overserviced * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
…-root * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
Co-authored-by: Bryan White <[email protected]>
Summary
Resolving a few blockers I've had mentioned in the codebase.
Issue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
golangci-lintversion in theMakefileto the latest version.