Bump cosmos-sdk to v0.50.10-pio-1 (from v0.50.7-pio-1).
#2175
Conversation
…ault node home directory.
…r signing context because the validation was failing because the custom msg signers funcs aren't getting propagated from the interface registry to the signing context.
… the previous commit, I also updated how we set the default node home directory to match the SDK's example (using one of their helpers).
…stuff applied (and will hopefully get merged and become v0.50.10-pio-1).
…s dependencies and there's no messages.
…Now and add line counts to the outputs. Also, remove the app/test_helpers.go filter since that file doesn't use .Now anymore.
…ry.Now(). The telemetry verision returns an empty Time if telemetry isn't enabled which is supposed to save on a bit a processing.
…t line for an event, and a space on the rest. This makes it easier to identify where one event starts and another ends. Also, update AssertEqualEvents to output the actual events when the check fails.
…vents that the bank module emits changed a little bit (for the better).
…e output of EventsToStrings.
WalkthroughThe pull request updates the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
I am not sure how much value the telemetry is actually providing on these endpoints ... they seem like they had this added back in the very beginning/early days of our protocol and yet I am not sure we have ever used or relied on their measurements.. More likely is the monitoring of overall block execution time and if there is an outlier then going into the block and seeing what it was doing and why that might be so time consuming....
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (17)
app/prefix.go (1)
13-15: LGTM with a minor suggestion for the comment.The addition of the
EnvPrefixconstant is well-documented and aligns with the PR objectives. It provides a centralized definition for the environment variable prefix, which can improve consistency across the application.Consider slightly modifying the comment to be more specific:
- // EnvPrefix is the prefix added to config/flag names to get its environment variable name. + // EnvPrefix is the prefix used for environment variable names associated with application configuration.This change more clearly indicates that the prefix is specifically for environment variables related to the application's configuration.
x/marker/abci.go (1)
Line range hint
18-53: Consider refining error handling and loggingWhile the changes in this PR are approved, there are a few suggestions for future improvements:
The error handling at the end of the function panics if an error occurs. In a blockchain context, panicking can be disruptive. Consider a more graceful error handling approach, such as returning an error from the
BeginBlockerfunction.The use of a closure with a mutable
errvariable could be improved for better readability and maintainability. Consider refactoring to use a named return error or a different iteration pattern.The logging of supply adjustments uses the
Errorlevel. If this is an expected operation, consider usingInfoorDebuglevel instead.Here's a potential refactoring for points 1 and 2:
func BeginBlocker(ctx sdk.Context, k keeper.Keeper, bk bankkeeper.Keeper) error { defer telemetry.ModuleMeasureSince(types.ModuleName, telemetry.Now(), telemetry.MetricKeyBeginBlocker) return k.IterateMarkers(ctx, func(record types.MarkerAccountI) error { // ... (rest of the logic) if err != nil { return err } return nil }) }This approach returns errors instead of panicking and simplifies the error handling within the iteration.
scripts/no-now-lint.sh (2)
21-39: LGTM: Well-implementedget_line_countfunctionThe new
get_line_countfunction is a robust solution for accurately counting lines, handling edge cases like empty strings or strings without trailing newlines. The detailed comments explaining why other methods are not suitable are particularly helpful.Consider adding a brief usage example in the function's comment to make it even more developer-friendly. For instance:
# Usage: count=$(get_line_count "$var")
45-70: LGTM: Improved import detection logicThe updates to the import detection logic effectively expand the script's scope to catch potential new libraries with
.Now()functions. Explicitly ignoring the vendor directory is a good practice.Consider adding a comment explaining the reasoning behind ignoring the vendor directory. For example:
# Ignore vendor directory as it's not under our control and may contain many false positivesThis would provide more context for future maintainers.
go.mod (1)
Line range hint
220-235: Consider addressing remaining TODOs and replacementsThere are several TODO comments and replacements in the file that might need attention in future updates. These include:
- Updating async-icq once a tag is created with specific changes.
- Updating or changing ibc-go once a tag is made with a fix for a specific issue.
- Removing deprecated jwt-go dependency.
- Addressing upstream vulnerabilities in gin-gonic/gin.
It would be beneficial to create issues or tasks to track these items for future maintenance and updates.
.changelog/add-change.sh (1)
328-328: LGTM! Consider adding a comment for clarity.The change to use
get-dep-changes.shfor the "dependencies" section is a good improvement. It aligns with the PR objective of updating the cosmos-sdk version and likely provides more accurate changelog entries for dependency updates.Consider adding a brief comment explaining the purpose of
get-dep-changes.shfor future maintainability:if [[ "$section" == "dependencies" ]]; then [[ -n "$verbose" ]] && printf 'Using get-dep-changes.sh for new entry.\n' - "${where_i_am}/get-dep-changes.sh" "$num_type_flag" "$num" --id "$id" --force + # Use get-dep-changes.sh to generate accurate changelog entries for dependency updates + "${where_i_am}/get-dep-changes.sh" "$num_type_flag" "$num" --id "$id" --force exit $? fiapp/sim_test.go (3)
234-236: LGTM! Consider adding a comment for clarity.The addition of the conditional check for
simcli.FlagSigverifyTxValueis appropriate and aligns with the PR objectives. It allows the simulation to run without transaction signature verification when needed.Consider adding a brief comment explaining the purpose of this condition for better code readability:
if !simcli.FlagSigverifyTxValue { + // Disable signature verification for transactions in simulation when flag is false app.SetNotSigverifyTx() }
597-599: LGTM! Crucial consistency in state determinism test.The addition of the conditional check for
simcli.FlagSigverifyTxValuein the TestAppStateDeterminism function maintains the consistency seen throughout the file. This change is particularly important in the context of state determinism testing, as it allows verification of deterministic behavior both with and without signature verification.Consider adding a test case that explicitly compares the state determinism with and without signature verification enabled. This could provide additional confidence in the application's behavior under different signature verification settings.
Line range hint
234-599: Overall assessment: Well-implemented and consistent changes.The modifications in this file consistently implement the conditional check for
simcli.FlagSigverifyTxValueacross all simulation test functions. These changes align well with the PR objectives and enhance the flexibility of the simulation tests by allowing them to run with or without signature verification.Key points:
- Consistency: The same pattern is applied uniformly across different test scenarios.
- Flexibility: Tests can now be run in both signature verification modes, increasing test coverage.
- Alignment: Changes support the new
SigverifyTxValuesims flag as intended.To further improve test coverage and confidence:
- Consider adding specific test cases that compare results with signature verification enabled and disabled.
- Document the expected behavior differences (if any) when running simulations with and without signature verification.
x/exchange/keeper/msg_server_test.go (8)
Line range hint
94-107: Simplify Generic Type Definitions for ReadabilityThe generic type definitions for
msgServerTestDefandmsgServerTestCaseuse placeholdersR,S, andFwithout clear context, which can reduce readability. Consider using more descriptive type parameter names to enhance code clarity.For example, you could rename the type parameters as follows:
-type msgServerTestDef[R any, S any, F any] struct { +type msgServerTestDef[Req any, Resp any, FollowupArgs any] struct { // ... } -type msgServerTestCase[R any, F any] struct { +type msgServerTestCase[Req any, FollowupArgs any] struct { // ... }
Line range hint
184-190: Handle Potential Panics inrunMsgServerTestCaseIn the
runMsgServerTestCasefunction, there is a defer function intended to restore the original context after the test execution. However, if any of the setup functions or the test itself panics, the deferred function might not execute as expected. Ensure that the test environment is correctly reset even if a panic occurs.Consider wrapping the test execution in a
deferwithrecover()to safely handle any panics:func runMsgServerTestCase[Req any, Resp any, FollowupArgs any](s *TestSuite, td msgServerTestDef[Req, Resp, FollowupArgs], tc msgServerTestCase[Req, FollowupArgs]) { s.T().Helper() origCtx := s.ctx defer func() { s.ctx = origCtx if r := recover(); r != nil { s.T().Errorf("Test case panicked: %v", r) } }() // ... rest of the test execution code }
Line range hint
679-693: Avoid Redundant Logging in TestsThe
getLogOutputmethod is being called within tests, which may not be necessary and could clutter test output, making it harder to diagnose failures.If the log output is not being used for assertions or test verifications, consider removing the call to reduce unnecessary processing:
- resp, err = td.endpoint(s.ctx, &tc.msg) - } - s.Require().NotPanicsf(testFunc, td.endpointName) - _ = s.getLogOutput(td.endpointName) + resp, err = td.endpoint(s.ctx, &tc.msg) + } + s.Require().NotPanicsf(testFunc, td.endpointName)
Line range hint
811-827: Consolidate Event Creation Helper FunctionsSeveral helper functions for event creation, such as
eventCoinSpent,eventCoinReceived, andeventTransfer, share similar structures. Consolidating them can reduce code duplication and improve maintainability.Consider creating a generic function to build events and use it across the specific event creation functions:
func (s *TestSuite) newEvent(eventType string, attributes map[string]string) sdk.Event { var attrs []abci.EventAttribute for k, v := range attributes { attrs = append(attrs, s.newAttr(k, v)) } return sdk.Event{ Type: eventType, Attributes: attrs, } }Then, update the specific event functions:
func (s *TestSuite) eventCoinSpent(spender sdk.AccAddress, amount string) sdk.Event { return s.newEvent("coin_spent", map[string]string{ "spender": spender.String(), "amount": amount, }) }
Line range hint
1293-1301: Check for Potential Data Races in Concurrent Test ExecutionIn
TestMsgServer_CreateAsk, the tests are being run usings.Run, which runs subtests. If any shared resources are modified without proper synchronization, this can lead to data races.Ensure that shared resources like
s.k,s.ctx, and any shared state are accessed safely. Consider using locks or redesigning the tests to avoid concurrent writes to shared state.
Line range hint
1675-1685: Correct Misuse of Error Handling in TestsIn the test case for
TestMsgServer_FillBids, the error messages are being checked usingexpInErr, but there is a misuse of error comparison that might lead to false positives.Ensure that error assertions are precise and do not rely on substring matching that could match unintended errors. Use
errors.Isor compare error types directly when possible.For example:
// Instead of checking substrings s.assertErrorContentsf(err, tc.expInErr, "%s error", td.endpointName) // Use a specific error assertion s.Require().ErrorIsf(err, expectedErrorType, "%s error", td.endpointName)
Line range hint
3104-3116: Use Table-Driven Tests for Parameter Update CasesIn
TestMsgServer_UpdateParams, multiple test cases check different scenarios of parameter updates. Using table-driven tests can improve test readability and scalability as new test cases are added.Refactor the test function to use a slice of test cases:
func (s *TestSuite) TestMsgServer_UpdateParams() { testCases := []struct { name string setup func() msg exchange.MsgUpdateParamsRequest expEvents sdk.Events }{ { /* test case definitions */ }, } for _, tc := range testCases { s.Run(tc.name, func() { if tc.setup != nil { tc.setup() } // ... test execution and assertions }) } }
Line range hint
1490-1502: Remove Redundant Type AssertionsIn
TestMsgServer_CreateAsk, within thefollowupfunction, there are explicit type assertions that may be unnecessary.If the types are known and assured by the context, you can remove redundant assertions to simplify the code:
- s.followup = func(_ *exchange.MsgCreateAskRequest, fargs followupArgs) { + s.followup = func(_, fargs followupArgs) { s.checkBalances(fargs.expBal) }Alternatively, if type safety is a concern, ensure that the type assertions are necessary and correctly implemented.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
- .changelog/add-change.sh (1 hunks)
- .changelog/unreleased/dependencies/2175-bump-sdk-to-50-10.md (1 hunks)
- .golangci.yml (1 hunks)
- app/app.go (4 hunks)
- app/prefix.go (1 hunks)
- app/sim_test.go (5 hunks)
- cmd/provenanced/cmd/root.go (1 hunks)
- go.mod (2 hunks)
- scripts/no-now-lint.sh (3 hunks)
- testutil/assertions/events.go (2 hunks)
- testutil/assertions/events_test.go (19 hunks)
- testutil/assertions/general_test.go (2 hunks)
- x/attribute/keeper/keeper.go (5 hunks)
- x/exchange/keeper/msg_server_test.go (13 hunks)
- x/marker/abci.go (1 hunks)
- x/marker/keeper/marker.go (16 hunks)
- x/metadata/keeper/msg_server.go (22 hunks)
- x/metadata/keeper/query_server.go (22 hunks)
✅ Files skipped from review due to trivial changes (3)
- .changelog/unreleased/dependencies/2175-bump-sdk-to-50-10.md
- x/metadata/keeper/msg_server.go
- x/metadata/keeper/query_server.go
🧰 Additional context used
🔇 Additional comments (56)
x/marker/abci.go (1)
16-16: Approved: Telemetry timing function updatedThe change from
time.Now()totelemetry.Now()is correct and aligns with the PR objectives. This update standardizes the telemetry timing functions across the codebase, potentially improving efficiency when telemetry is not enabled.testutil/assertions/events.go (3)
26-28: Improved event string formattingThe changes in the
EventsToStringsfunction enhance the readability of the output by adding a ">" character to the first line of each event. This modification makes it easier to distinguish between different events when multiple events are displayed.The implementation is correct and improves the overall formatting without affecting the function's core functionality.
85-90: Enhanced debugging for failed event assertionsThe changes in the
AssertEqualEventsfunction significantly improve the debugging experience when event assertions fail. By logging the actual events when the assertion fails, developers can more easily identify and understand the discrepancies between expected and actual events.This modification is particularly useful in complex test scenarios where manually comparing large sets of events can be time-consuming and error-prone. The implementation is correct and maintains the original function behavior while adding valuable debugging information.
Line range hint
1-190: Summary of changes in testutil/assertions/events.goThe modifications in this file enhance the functionality of event-related test utilities:
- Improved formatting in
EventsToStringsfor better readability of multiple events.- Enhanced debugging capabilities in
AssertEqualEventsby logging actual events on assertion failures.These changes align with the PR objectives and will likely improve the development experience when working with event-related tests. The implementations are correct and maintain backwards compatibility while adding valuable features.
testutil/assertions/general_test.go (3)
83-83: Improved error message clarity.The addition of the expected value (
exp) to the error message enhances the clarity of test output. This change will make it easier to diagnose failed assertions by providing more context about what was being searched for in the output.
98-98: Consistent improvement in error message clarity.This change mirrors the improvement made in the
assertMockRunAssertResultfunction, maintaining consistency across assert and require functions. It enhances the clarity of test output for require assertions, making it easier to identify the specific expected value when a test fails.
83-83: Summary: Enhanced test diagnosticsThese changes collectively improve the diagnostic capabilities of the test assertions by providing more detailed error messages. This aligns well with the PR's objective of enhancing test output and will make it easier for developers to identify and fix issues in failed tests.
Also applies to: 98-98
scripts/no-now-lint.sh (3)
2-4: LGTM: Updated comments accurately reflect script changesThe modifications to the comments effectively communicate the expanded scope of the script, now checking for all
.Now()functions instead of justtime.Now(). The addition of known libraries with.Now()functions provides valuable context for developers.Also applies to: 16-19
Line range hint
73-95: LGTM: Enhanced.Now()usage detectionThe updates to the
.Now()usage detection logic effectively broaden the script's scope to catch all relevant.Now()calls from imported aliases. The existing logic for ignoring comments is maintained, ensuring accurate detection.
Line range hint
97-125: LGTM: Improved filtering logic and output formattingThe updates to the filtering logic and output formatting enhance the script's clarity and usefulness. The addition of counts in the verbose output provides valuable information to users.
Could you please provide more context about the new filter for the
x/marker/client/cli/tx.gofile? It would be helpful to understand the specific use case this addresses and ensure it doesn't inadvertently ignore any problematic uses of.Now().go.mod (3)
13-13: LGTM: Minor version update for cosmossdk.io/storeThe update from v1.1.0 to v1.1.1 for
cosmossdk.io/storeis a minor version increment, which typically includes bug fixes and small improvements. This change aligns with the PR objective of updating dependencies.
217-217: Verify the use of provenance-io fork for cosmos-sdkThe update to
v0.50.10-pio-1aligns with the main objective of this PR. However, it's important to ensure that using theprovenance-iofork of cosmos-sdk instead of the main repository is intentional and necessary.Could you please confirm if there are specific reasons for using the
provenance-iofork of cosmos-sdk? Are there any custom changes or patches in this fork that are required for the project?
Line range hint
212-213: Confirm long-term plan for wasmd fork usageThe replacement of
github.com/CosmWasm/wasmdwithgithub.com/provenance-io/wasmd v0.52.0-pio-1addresses issue #1414. While this solves an immediate problem, it's important to consider the long-term implications.
- Could you provide more details about issue Wasm storage via new gov proposal functionality not supported #1414 and why it requires using a fork of wasmd?
- Is there a plan to contribute these changes back to the main wasmd repository?
- How will this fork be maintained to keep up with upstream changes and security updates?
cmd/provenanced/cmd/root.go (3)
81-81: LGTM: Improved flexibility in environment prefix configurationThe change from a hardcoded "PIO" to
app.EnvPrefixenhances the flexibility of the application's configuration. This is a good practice as it centralizes the environment prefix definition and makes it easier to maintain or modify in the future.
Line range hint
156-196: Request clarification oninitRootCmdchangesThe AI summary mentioned modifications to the
initRootCmdfunction to ensure that the command structure remains intact. However, I don't see any changes in the provided code snippet for this function. Could you please clarify:
- Were there any changes made to the
initRootCmdfunction?- If changes were made, what was the purpose and impact of these changes?
- If no changes were made, should we update the PR description to reflect this?
Let's check for any changes in the
initRootCmdfunction:#!/bin/bash # Search for changes in initRootCmd function git diff HEAD^ HEAD -- cmd/provenanced/cmd/root.go | grep -A 20 'func initRootCmd'
Line range hint
153-154: Approve new configuration flags, but request usage detailsThe addition of
CustomDenomFlagandCustomMsgFeeFloorPriceFlagenhances the application's configurability, which is beneficial for supporting different network setups and economic models. This is a positive change.However, could you please provide more information on:
- How these flags are used within the application?
- Are there any validation mechanisms in place for these flags?
- Are there any default values or constraints for these flags that should be documented in the help text?
To ensure proper usage of these new flags, let's check for their implementation:
app/sim_test.go (3)
281-283: LGTM! Consistent implementation across test functions.The addition of the conditional check for
simcli.FlagSigverifyTxValueis consistent with the previous test function. This consistency is good for maintainability and ensures that all simulation tests behave the same way with respect to signature verification.
337-339: LGTM! Maintaining consistency across test functions.The addition of the conditional check for
simcli.FlagSigverifyTxValueis consistent with the previous test functions. This continued consistency across different types of simulation tests (full app, simple, and import/export) ensures uniform behavior with respect to signature verification.
469-471: LGTM! Consistency maintained in TestAppSimulationAfterImport.The addition of the conditional check for
simcli.FlagSigverifyTxValueremains consistent with all previous test functions. This uniformity across different simulation scenarios (including after import) ensures that the signature verification behavior is consistent throughout the testing suite.x/attribute/keeper/keeper.go (7)
89-89: Telemetry measurement standardized.The change from
time.Now()totelemetry.Now()aligns with the goal of standardizing telemetry measurement calls. This should improve the accuracy of telemetry measurements when enabled.
97-97: Telemetry measurement standardized.The change from
time.Now()totelemetry.Now()is consistent with the goal of standardizing telemetry measurement calls. This should improve the accuracy of telemetry measurements when enabled.
105-105: Telemetry measurement standardized.The replacement of
time.Now()withtelemetry.Now()aligns with the goal of standardizing telemetry measurement calls. This change should enhance the accuracy of telemetry measurements when enabled.
139-139: Telemetry measurement standardized.The substitution of
time.Now()withtelemetry.Now()is in line with the objective of standardizing telemetry measurement calls. This modification should improve the precision of telemetry measurements when enabled.
218-218: Telemetry measurement standardized.The change from
time.Now()totelemetry.Now()adheres to the goal of standardizing telemetry measurement calls. This alteration should enhance the accuracy of telemetry measurements when enabled.
301-301: Telemetry measurement standardized.The replacement of
time.Now()withtelemetry.Now()is consistent with the objective of standardizing telemetry measurement calls. This change should improve the precision of telemetry measurements when enabled.
374-374: Telemetry measurement standardized across all functions.The change from
time.Now()totelemetry.Now()in this function, as well as in all previously reviewed functions (GetAllAttributes,GetAllAttributesAddr,GetAttributes,SetAttribute,UpdateAttribute,UpdateAttributeExpiration), consistently implements the standardization of telemetry measurement calls.This systematic update across the keeper's methods should result in more accurate and efficient telemetry measurements when telemetry is enabled, without affecting the core functionality of the attribute management system.
testutil/assertions/events_test.go (8)
116-117: LGTM: Improved output formattingThe addition of the
>character at the beginning of the output string enhances the visual distinction of event entries. This change is consistent with the modifications described in the summary and improves readability.
124-129: LGTM: Consistent formatting improvementThe addition of the
>character at the beginning of each event's output string is consistent and improves the visual separation between different events. This change enhances the readability of the test output, making it easier to distinguish between multiple events.
328-329: LGTM: Consistent formatting in test casesThe addition of the
>character at the beginning of the actual output string in the test case is consistent with the previous modifications. This change improves the visual distinction of the actual output, making it easier to compare expected and actual results in the test cases.
338-340: LGTM: Consistent formatting in expected outputThe addition of the
>character at the beginning of the expected output string in the test case maintains consistency with the previous modifications. This change enhances the visual distinction of the expected output, facilitating easier comparison between expected and actual results in the test cases.
355-358: LGTM: Consistent formatting across various test scenariosThe addition of the
>character at the beginning of the output strings for both expected and actual results is consistent across various test scenarios. These changes improve the visual distinction of the output strings in different cases, including scenarios with different attribute keys, values, and indexed attributes. The uniform application of this formatting enhancement across multiple test cases improves overall readability and consistency.Also applies to: 367-370, 379-382, 391-394
403-405: LGTM: Consistent formatting in complex test scenariosThe addition of the
>character at the beginning of the output strings for both expected and actual results is consistently applied in more complex test scenarios. These changes enhance the visual distinction of the output strings in cases involving multiple attributes and events. The uniform application of this formatting improvement across various complex test cases contributes to better readability and easier comparison of test results.Also applies to: 414-416, 425-430
442-444: LGTM: Comprehensive and consistent formatting improvementsThe addition of the
>character at the beginning of the output strings for events is consistently applied across various test functions and scenarios throughout the file. These changes significantly enhance the visual distinction of event outputs in a wide range of test cases, including:
- Different event types and attributes
- Scenarios with missing or extra events
- Comparisons between expected and actual results
- Complex cases with multiple events and attributes
This comprehensive update improves the overall readability of the test output, making it easier to identify and compare events in the test results. The consistency of these changes across the entire test suite is commendable and will contribute to better maintainability and debugging of the tests.
Also applies to: 456-458, 485-490, 644-648, 757-761, 781-785, 805-809, 830-834
Line range hint
1-909: Overall assessment: Consistent and beneficial formatting improvementsThis review has covered all the changes made to the
testutil/assertions/events_test.gofile. The modifications consistently add a>character to the beginning of event output strings across various test functions and scenarios. These changes:
- Enhance the visual distinction of event entries in test outputs
- Improve readability and ease of comparison between expected and actual results
- Maintain consistency throughout the entire test suite
- Align with the objectives stated in the PR summary
The uniform application of these formatting improvements across different test cases and functions will contribute to better maintainability and easier debugging of the test suite. No issues or concerns were identified during this review.
x/marker/keeper/marker.go (16)
20-20: LGTM: Telemetry measurement updatedThe change from
time.Now()totelemetry.Now()is consistent with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
38-38: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous function and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
127-127: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
172-172: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
215-215: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
255-255: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
304-304: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
344-344: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
370-370: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
407-407: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
468-468: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
519-519: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
567-567: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
625-625: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
812-812: LGTM: Telemetry measurement updated consistentlyThe change from
time.Now()totelemetry.Now()is consistent with the previous functions and aligns with the goal of improving telemetry measurements. This modification should provide more accurate timing when telemetry is enabled, without affecting the function's core logic.
Line range hint
20-812: Overall assessment: Consistent telemetry improvementsThe changes in this file are uniform and focused on improving telemetry measurements by replacing
time.Now()withtelemetry.Now()across multiple functions. This modification should provide more accurate timing when telemetry is enabled, without affecting the core logic of any function. The consistency of these changes across the file is commendable and aligns well with the goal of enhancing performance monitoring capabilities.Key points:
- All changes are consistent and isolated to telemetry measurements.
- No modifications to the logic or structure of the functions.
- The changes should improve the accuracy of performance measurements when telemetry is enabled.
These updates are well-implemented and should contribute to better performance monitoring of the marker-related operations in the Cosmos SDK application.
app/app.go (4)
25-25: Addclienthelpersimport for helper functionsThe addition of the
clienthelpersimport is appropriate for accessing helper functions used in the code, such asGetNodeHomeDirectory.
313-317: InitializeDefaultNodeHomeusingclienthelpersThe updated initialization of
DefaultNodeHomeusingclienthelpers.GetNodeHomeDirectory("Provenance")simplifies the logic and leverages theclienthelperspackage effectively. Settingclienthelpers.EnvPrefix = EnvPrefixensures the environment variable prefix is correctly applied.Error handling is appropriate, with a panic on failure to determine the node home directory.
345-352: ConfiguretxConfigwith custom signing optionsThe introduction of
txConfigOptsand the use ofauthtx.NewTxConfigWithOptionsallow for flexible transaction configuration with customEnabledSignModesandSigningOptions. This ensures that transaction signing is properly configured.Error handling is appropriate by panicking on initialization failure.
457-459: Validate signing context to ensure correct configurationIncluding
txConfig.SigningContext().Validate()ensures that the signing context is properly set up, catching any misconfigurations early in the initialization process. Panicking on error is acceptable here to prevent the application from running with invalid settings.x/exchange/keeper/msg_server_test.go (2)
Line range hint
3115-3123: Ensure Proper Handling of Default ParametersWhen updating params in
TestMsgServer_UpdateParams, ensure that the default parameters are handled correctly, and no unintended changes are introduced.Review the parameter validation logic to ensure that updating with default values does not cause any side effects.
#!/bin/bash # Description: Verify that default params are handled correctly rg --type go 'func\s+ValidateParams' x/exchange/keeper/params.go -A 20Check the validation rules and update the tests if necessary.
Line range hint
2765-2772: Validate Error Messages for ConsistencyIn the test cases for
TestMsgServer_MarketCommitmentSettle, the error messages inexpInErrmight not reflect the actual errors thrown by the system if error messages change in the future.Run the following script to extract all error messages related to
MarketCommitmentSettleand ensure that the test cases use the correct and up-to-date error messages.Review the output and update the test cases accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .golangci.yml (1 hunks)
- app/app.go (4 hunks)
🧰 Additional context used
📓 Learnings (2)
.golangci.yml (1)
Learnt from: SpicyLemon PR: provenance-io/provenance#2175 File: .golangci.yml:87-87 Timestamp: 2024-10-10T23:08:15.256Z Learning: The `depguard` definitions in `.golangci.yml` are prefixes (unless they end with `$`), so the entry `- cosmossdk.io/client/v2` covers imports from `cosmossdk.io/client/v2` and its subpackages like `autocli`. Changing imports from `cosmossdk.io/client/v2/autocli` to `cosmossdk.io/client/v2` in `app/app.go` would result in unresolved references because the required code resides in the `autocli` package.app/app.go (1)
Learnt from: SpicyLemon PR: provenance-io/provenance#2175 File: app/app.go:451-452 Timestamp: 2024-10-10T23:08:46.079Z Learning: In `app/app.go` of the Provenance Blockchain (Go), the `txConfig` needs to be initialized in two stages because early components depend on a partially completed `txConfig`. The full setup of `txConfig` requires the `BankKeeper`, which isn't available until later in the initialization process. Therefore, `txConfig` is first initialized with essential options and then finalized after `BankKeeper` is created.
🔇 Additional comments (3)
.golangci.yml (1)
87-87: LGTM: Depguard rule updated to align with new cosmos-sdk versionThe change from
cosmossdk.io/client/v2/autoclitocosmossdk.io/client/v2in the depguard rules aligns with the cosmos-sdk version update tov0.50.10-pio-1mentioned in the PR objectives. This modification allows for broader import capabilities from thev2package.Important notes:
- This change doesn't require modifications to import statements in the actual code, as the
depguarddefinitions are prefixes.- The new rule will cover
cosmossdk.io/client/v2/autocliand other subpackages undercosmossdk.io/client/v2.app/app.go (2)
313-315: Proper error handling in initializationThe error handling for
DefaultNodeHome, err = clienthelpers.GetNodeHomeDirectory("Provenance")is appropriate. The code correctly panics if an error occurs during initialization, ensuring that any issues are caught early in the startup process.
447-455: Re-initialization of 'txConfig' is appropriateThe two-stage initialization of
txConfigis necessary due to dependencies onapp.BankKeeper. The initialtxConfigprovides essential options required by early components, while the complete setup occurs afterapp.BankKeeperis available. This approach aligns with the application's initialization requirements.
Description
This PR does the following:
v0.50.10-pio-1(fromv0.50.7-pio-1). See also: Bring in SDK changes up to v0.50.10 and Mark v0.50.10-pio-1. cosmos-sdk#612.GetNodeHomeDirectoryhelper to set theDefaultNodeHomepath. Now,DefaultNodeHomewill have a value that reflects the--homeflag orPIO_HOMEenv arg and defaults to theos.UserConfigDir()otherwise.txConfigis created inapp.Newbecause the custom signing funcs weren't properly being propagated to it (and I added a call to.Validate()like the SDK's example, that was causing apanicwithout the tweak).SigverifyTxValuesims flag..changelog/add-change.sh. When the section is "dependencies" and no messages are provided, it now correctly callsget-dep-changes.shinstead of erroneously callingget-valid-sections.sh.no-now-lint.shscript to also look for uses oftelemetry.Now().telemetry.Now()(fromtime.Now()). Thetelemetry.Now()is supposed to be more efficient when telemetry isn't enabled.Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/).godoccomments..changelog/unreleased(see Adding Changes).Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores