Conversation
joe-bowman
requested review from
ajansari95,
arhamchordia and
faddat
as code owners
WalkthroughThis pull request updates dependency management across multiple Go modules and refines control flow in key functions. The main Changes
Sequence Diagram(s)sequenceDiagram
participant Keeper
participant Query
Keeper->>Query: Retrieve queryInfo.Period
alt Period is negative (periodic query)
Keeper->>Emitter: Process periodic query emission
else
Keeper->>Bypass: Skip emission for non-periodic query
end
sequenceDiagram
participant Handler
participant Record
Handler->>Record: Check if current time > CompletionTime and CompletionTime ≠ zero
alt Condition met
Handler->>Record: Delete matured unbonding record
else
alt Epoch 258-261 and missing record
Handler->>Record: Create temporary unbonding record (no txn hash)
else
Handler->>Handler: No action taken
end
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
x/interchainstaking/keeper/ibc_packet_handlers.go (1)
791-806: Verify the temporary fix for epochs 258-261.This code block adds a temporary fix for a specific bug that occurred during epochs 258-261 where unbonding records were not found. The fix creates a stub unbonding record without a related txhash, allowing the unbond transaction to be rescheduled in the next epoch.
Consider adding a TODO comment to track the removal of this temporary fix once all affected epochs are processed.
icq-relayer/go.mod (15)
15-15: Dependency Update: Quicksilver v1.7.6
The module version forgithub.com/quicksilver-zone/quicksilverhas been updated to v1.7.6. Please verify that this update is compatible with all dependent modules within the project.
25-25: Dependency Update: cloud.google.com/go/auth v0.14.1
The dependency forcloud.google.com/go/authhas been updated to v0.14.1. Ensure that this upgrade does not affect authentication workflows or introduce incompatibilities in related modules.
118-118: New Indirect Dependency: HashiCorp go-uuid v1.0.3
A new indirect dependencygithub.com/hashicorp/go-uuid(v1.0.3) has been added. Confirm that this dependency is necessary for the intended functionality and that it does not introduce any conflicts.
143-143: Dependency Update: moby/sys/user v0.3.0
The dependencygithub.com/moby/sys/userhas been updated to v0.3.0. Verify that any functionality relying on system user operations works as expected with the updated version.
146-146: Dependency Update: onsi/gomega v1.36.2
The testing frameworkgithub.com/onsi/gomegahas been updated to v1.36.2. Please rerun the full test suite to ensure that no regressions or incompatibilities have been introduced.
164-164: Dependency Update: spf13/afero v1.12.0
Thespf13/aferodependency has been upgraded to v1.12.0. This update may include bug fixes or improvements in file system abstractions. Ensure compatibility with file I/O operations in the project.
184-184: Dependency Update: OpenTelemetry SDK v1.34.0
The OpenTelemetry SDKgo.opentelemetry.io/otel/sdkis updated to v1.34.0. Please confirm that telemetry instrumentation and related integrations continue to function correctly after this update.
188-188: Dependency Update: golang.org/x/crypto v0.33.0
The cryptography packagegolang.org/x/cryptohas been updated to v0.33.0. Verify that all cryptographic operations remain secure and that there are no breaking changes affecting crypto functions.
190-190: Dependency Update: golang.org/x/net v0.35.0
The networking librarygolang.org/x/netis updated to v0.35.0. Ensure that network operations and protocols continue to behave correctly with the new version.
192-192: Dependency Update: golang.org/x/sync v0.11.0
Thegolang.org/x/syncpackage has been updated to v0.11.0. Please verify that synchronization primitives and concurrent routines function as intended.
193-193: Dependency Update: golang.org/x/sys v0.30.0
The system interface packagegolang.org/x/sysis updated to v0.30.0. This update could affect low-level system calls; ensure that any such interactions in the project remain stable.
194-194: Dependency Update: golang.org/x/term v0.29.0
The terminal handling librarygolang.org/x/termhas been updated to v0.29.0. It is advisable to test any console-based operations to confirm they are unaffected by this update.
195-195: Dependency Update: golang.org/x/text v0.22.0
The text processing dependencygolang.org/x/textis now at v0.22.0. Check for any changes in string processing or formatting that might impact the application's output.
197-197: Dependency Update: google.golang.org/api v0.220.0
The Google API packagegoogle.golang.org/apihas been updated to v0.220.0. Please validate that all integrations with Google services work correctly with this new version.
202-202: Dependency Update: google.golang.org/protobuf v1.36.5
The protobuf librarygoogle.golang.org/protobufis now at v1.36.5. Ensure that Protobuf serialization and deserialization remain compatible with existing definitions and data formats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumicq-relayer/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.mod(0 hunks)icq-relayer/go.mod(7 hunks)x/interchainquery/keeper/abci.go(1 hunks)x/interchainstaking/keeper/ibc_packet_handlers.go(3 hunks)
💤 Files with no reviewable changes (1)
- go.mod
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: Analyze
- GitHub Check: Analyze
🔇 Additional comments (4)
x/interchainquery/keeper/abci.go (1)
25-25: Verify the inverted logic for periodic query handling.The condition has been inverted from
!queryInfo.Period.IsNegative()toqueryInfo.Period.IsNegative(). This changes the behavior to process non-periodic queries instead of periodic ones, which aligns with the PR objectives for fixing GC of stale ICQ records.x/interchainstaking/keeper/ibc_packet_handlers.go (2)
563-563: LGTM! Additional check prevents deletion of records with zero completion time.The added condition
!record.CompletionTime.Equal(time.Time{})ensures that unbonding records with a zero completion time are not erroneously deleted, which aligns with the PR objectives.
827-828: LGTM! Setting completion time for unbonding record.The changes properly update the completion time for the unbonding record and persist it to storage.
icq-relayer/go.mod (1)
137-137: Dependency Update: mattn/go-colorable v0.1.14
The version ofgithub.com/mattn/go-colorablehas been updated to v0.1.14. This change should help with colored terminal output; please ensure downstream consumers render colors correctly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1821 +/- ##
==========================================
+ Coverage 62.95% 63.07% +0.11%
==========================================
Files 172 172
Lines 15537 15549 +12
==========================================
+ Hits 9781 9807 +26
+ Misses 4945 4933 -12
+ Partials 811 809 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
third-party-chains/osmosis-types/osmoutils/params_validation.go (1)
26-26: Consider preserving error context in the error message.The change from
fmt.Errorftoerrors.Newloses valuable debugging information about which address was invalid. Consider wrapping the original error to preserve context:- return errors.New("invalid address") + return fmt.Errorf("invalid address %q: %w", a, err)x/supply/keeper/grpc_query.go (1)
45-45: Consider defining the reused error message as a constant.Since the "endpoint disabled" error is used in multiple methods, consider defining it as a package-level constant:
+var errEndpointDisabled = errors.New("endpoint disabled") + func (q Querier) Supply(c context.Context, _ *types.QuerySupplyRequest) (*types.QuerySupplyResponse, error) { // ... - return nil, errors.New("endpoint disabled") + return nil, errEndpointDisabled } func (q Querier) TopN(c context.Context, req *types.QueryTopNRequest) (*types.QueryTopNResponse, error) { // ... - return nil, errors.New("endpoint disabled") + return nil, errEndpointDisabled }Also applies to: 57-57
app/tps_counter.go (1)
115-115: Consider improving error handling for overflow condition.While the change to
errors.Newis appropriate, this error leads to a panic in thestartmethod. Consider handling the overflow condition more gracefully.- if err == nil { - nTxn += nSuccess - } else { - panic(err) - } + if err != nil { + tpc.logger.Error("Error recording transaction value", "error", err) + continue + } + nTxn += nSuccessthird-party-chains/osmosis-types/osmoutils/coin_helper.go (1)
13-13: Consider extracting the common error message.The same error message is duplicated across three functions. Consider extracting it to a package-level constant to improve maintainability.
+const errUnequalLengthArrays = "DecCoin arrays must be of equal length to be subtracted" func SubDecCoinArrays(decCoinsArrayA []sdk.DecCoins, decCoinsArrayB []sdk.DecCoins) ([]sdk.DecCoins, error) { if len(decCoinsArrayA) != len(decCoinsArrayB) { - return []sdk.DecCoins{}, errors.New("DecCoin arrays must be of equal length to be subtracted") + return []sdk.DecCoins{}, errors.New(errUnequalLengthArrays) }Also applies to: 29-29, 45-45
x/interchainstaking/types/redemptions.go (1)
86-86: Fix duplicate error code in error message.The error message at line 181 uses error code "(2)" which is already used at line 137. This could cause confusion when debugging. Consider using "(3)" instead.
Apply this diff to fix the error code:
- return map[string]math.Int{}, errors.New("input is unexpectedly negative (2), aborting") + return map[string]math.Int{}, errors.New("input is unexpectedly negative (3), aborting")Also applies to: 137-137, 181-181
.golangci.yml (1)
26-27: Addition of "usetesting" linter.
The new linter "usetesting" has been added to the enabled linters list to potentially improve test-related validations. Ensure that this linter integrates well with our existing development practices and that its checks don’t lead to unexpected warnings or false positives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
.github/workflows/golangci.yml(1 hunks).golangci.yml(1 hunks)app/metrics.go(2 hunks)app/tps_counter.go(2 hunks)app/upgrades/types.go(1 hunks)app/upgrades/upgrades.go(1 hunks)icq-relayer/pkg/runner/run.go(4 hunks)icq-relayer/pkg/types/client.go(2 hunks)test/simulation/simtypes/account.go(1 hunks)third-party-chains/osmosis-types/concentrated-liquidity/keys.go(2 hunks)third-party-chains/osmosis-types/concentrated-liquidity/math/tick.go(1 hunks)third-party-chains/osmosis-types/concentrated-liquidity/model/msgs.go(2 hunks)third-party-chains/osmosis-types/concentrated-liquidity/params.go(4 hunks)third-party-chains/osmosis-types/gamm/pool-models/balancer/pool.go(4 hunks)third-party-chains/osmosis-types/gamm/pool-models/balancer/pool_asset.go(2 hunks)third-party-chains/osmosis-types/lockup/msgs.go(4 hunks)third-party-chains/osmosis-types/osmomath/math.go(3 hunks)third-party-chains/osmosis-types/osmoutils/coin_helper.go(4 hunks)third-party-chains/osmosis-types/osmoutils/params_validation.go(2 hunks)third-party-chains/osmosis-types/osmoutils/parse.go(2 hunks)third-party-chains/osmosis-types/osmoutils/partialord/internal/dag/dag.go(3 hunks)utils/coins.go(2 hunks)utils/proofs/proofs.go(4 hunks)x/interchainstaking/client/cli/query.go(2 hunks)x/interchainstaking/keeper/ibc_packet_handlers.go(5 hunks)x/interchainstaking/keeper/ibc_packet_handlers_test.go(2 hunks)x/interchainstaking/keeper/msg_server.go(2 hunks)x/interchainstaking/keeper/redemptions.go(1 hunks)x/interchainstaking/keeper/withdrawal_record.go(5 hunks)x/interchainstaking/types/delegation.go(1 hunks)x/interchainstaking/types/params.go(1 hunks)x/interchainstaking/types/redemptions.go(4 hunks)x/participationrewards/keeper/distribution.go(2 hunks)x/supply/keeper/grpc_query.go(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- third-party-chains/osmosis-types/concentrated-liquidity/math/tick.go
- x/interchainstaking/keeper/redemptions.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/ibc_packet_handlers.go
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: test quicksilver (darwin, arm64)
- GitHub Check: test quicksilver (amd64, windows)
- GitHub Check: test quicksilver (amd64, linux)
- GitHub Check: build quicksilver (darwin, arm64)
- GitHub Check: test (1.23.3, macos-latest)
- GitHub Check: build quicksilver (amd64, darwin)
- GitHub Check: build quicksilver (amd64, windows)
- GitHub Check: test (1.23.3, ubuntu-latest)
- GitHub Check: Analyze
- GitHub Check: build quicksilver (amd64, linux)
- GitHub Check: lint
- GitHub Check: test (1.23.3, macos-latest)
- GitHub Check: test (1.23.3, ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (34)
third-party-chains/osmosis-types/osmoutils/parse.go (1)
27-27: LGTM!The error messages are clear and descriptive. Since they don't require dynamic formatting, using
errors.Newis appropriate here.Also applies to: 31-31
utils/coins.go (1)
22-22: LGTM!The error message is clear and appropriate for the empty denom case. The remaining
fmt.Errorfusage is correctly preserved where dynamic values are needed.app/metrics.go (1)
38-38: LGTM! Error handling improvement.The change from
fmt.Errorftoerrors.Newfor static error messages aligns with best practices and the codebase-wide standardization effort.utils/proofs/proofs.go (1)
32-32: LGTM! Consistent error handling improvements.The changes appropriately use:
errors.Newfor static error messagesfmt.Errorffor dynamic error messages with variablesThis maintains good error handling practices while improving consistency.
Also applies to: 53-53, 57-57, 77-77
third-party-chains/osmosis-types/concentrated-liquidity/model/msgs.go (1)
50-50: LGTM! Appropriate error handling change.The change to
errors.Newfor the static error message is appropriate, while maintainingfmt.Errorffor error messages with dynamic content.third-party-chains/osmosis-types/gamm/pool-models/balancer/pool_asset.go (1)
27-27: LGTM! Error handling improvement.The change to
errors.Newis appropriate here as the error message is static and doesn't require formatting.x/interchainstaking/types/params.go (1)
171-171: LGTM! Error handling improvement.The change to
errors.Newis appropriate here as the error message for nil value is static and doesn't require formatting.x/interchainstaking/types/delegation.go (1)
148-148: LGTM! Standardized error handling.The changes simplify error creation by using
errors.Newinstead offmt.Errorffor static error messages, which is a good practice as it's more efficient when formatting is not needed.Also applies to: 151-151
third-party-chains/osmosis-types/lockup/msgs.go (1)
53-53: LGTM! Standardized error handling.The changes consistently use
errors.Newfor static error messages across multipleValidateBasicmethods, improving code consistency and efficiency.Also applies to: 125-125, 157-157
test/simulation/simtypes/account.go (1)
220-220: LGTM! Standardized error handling.The change simplifies error creation by using
errors.Newfor the static error message, consistent with the project's error handling standards.third-party-chains/osmosis-types/osmoutils/partialord/internal/dag/dag.go (2)
64-65: LGTM! Appropriate use of errors.New for static error messages.The change from
fmt.Errorftoerrors.Newfor static error messages is a good improvement. This is more efficient as it avoids unnecessary string formatting.Also applies to: 78-79
67-68: LGTM! Consistent error handling improvement.The change to
errors.Newmaintains consistency with the other error handling changes.third-party-chains/osmosis-types/concentrated-liquidity/params.go (1)
122-123: LGTM! Appropriate use of errors.New for static error messages.The changes from
fmt.Errorftoerrors.Newfor static error messages in validation functions are good improvements. This is more efficient as it avoids unnecessary string formatting.Also applies to: 175-176, 231-232
x/participationrewards/keeper/distribution.go (1)
72-74: LGTM! Appropriate use of errors.New for static error messages.The changes from
fmt.Errorftoerrors.Newfor static error messages in pool data validation are good improvements. This is more efficient as it avoids unnecessary string formatting.Also applies to: 133-135
x/interchainstaking/keeper/withdrawal_record.go (2)
56-58: LGTM! Appropriate use of errors.New for validation error messages.The changes from
fmt.Errorftoerrors.Newfor static error messages in withdrawal record validation are good improvements. This is more efficient as it avoids unnecessary string formatting.Also applies to: 90-92, 94-96
289-291: LGTM! Appropriate use of errors.New for overflow error messages.The changes from
fmt.Errorftoerrors.Newfor static error messages in slashing event handling are good improvements. The error messages clearly indicate potential overflow conditions.Also applies to: 305-307
third-party-chains/osmosis-types/concentrated-liquidity/keys.go (1)
274-274: LGTM! Appropriate use oferrors.New.The change from
fmt.Errorftoerrors.Newis correct here as these are static error messages without any variable interpolation.Also applies to: 278-278
x/interchainstaking/client/cli/query.go (1)
456-456: LGTM! Appropriate use oferrors.New.The change from
fmt.Errorftoerrors.Newis correct here as this is a static error message without any variable interpolation.x/interchainstaking/keeper/msg_server.go (1)
37-37: LGTM! Appropriate use oferrors.New.The changes from
fmt.Errorftoerrors.Neware correct here as these are static error messages without any variable interpolation.Also applies to: 190-190, 195-195, 199-199, 202-202
icq-relayer/pkg/types/client.go (1)
157-157: LGTM! Appropriate use oferrors.New.The changes from
fmt.Errorftoerrors.Neware correct here as these are static error messages without any variable interpolation.Also applies to: 453-453, 457-457, 461-461
icq-relayer/pkg/runner/run.go (3)
6-6: LGTM!The addition of the
errorspackage import is necessary for the subsequent error handling changes.
376-376: LGTM!The change from
fmt.Errorftoerrors.Newis appropriate here as the error message is static and doesn't require formatting.
532-532: LGTM!The change from
fmt.Errorftoerrors.Newis appropriate here as the error message is static and doesn't require formatting.third-party-chains/osmosis-types/gamm/pool-models/balancer/pool.go (4)
145-145: LGTM!The change from
fmt.Errorftoerrors.Newis appropriate here as the error message is static and doesn't require formatting.
154-154: LGTM!The change from
fmt.Errorftoerrors.Newis appropriate here as the error message is static and doesn't require formatting.
226-226: LGTM!The change from
fmt.Errorftoerrors.Newis appropriate here as the error message is static and doesn't require formatting.
305-305: LGTM!The change from
fmt.Errorftoerrors.Newis appropriate here as the error message is static and doesn't require formatting.x/interchainstaking/keeper/ibc_packet_handlers_test.go (4)
4983-5007: LGTM! Test verifies that future unbonding records are preserved.The test ensures that unbonding records with a future completion time are not deleted when
HandleMaturedUnbondingsis called, which aligns with the PR's GC fixes.
5009-5033: LGTM! Test verifies that expired unbonding records are deleted.The test confirms that unbonding records with a completion time in the past are properly deleted by the GC, which directly addresses the PR's focus on fixing GC issues.
5035-5059: LGTM! Test verifies that records with nil completion time are preserved.The test validates that unbonding records with a zero completion time are not deleted, which aligns with the PR's fix for the GC of unbonding records that have a nil time.
5061-5085: LGTM! Test verifies creation of new unbonding records.The test ensures that a new unbonding record is correctly created with the expected properties when no existing record is found for epoch 259. This provides coverage for the record creation logic.
.github/workflows/golangci.yml (1)
31-32: Update golangci-lint version to v1.64.5.
This update ensures that we are leveraging the latest improvements, bug fixes, and features from golangci-lint. Please verify that this newer version produces consistent linting results within our CI pipeline and that any changes in lint warnings are duly addressed.app/upgrades/upgrades.go (1)
32-32:Details
✅ Verification successful
Verify if NoOpHandler is appropriate for GC fixes.
The PR objectives mention fixes for garbage collection of stale ICQ records and unbonding records. Please verify if these fixes require state manipulation, as using
NoOpHandlerimplies no state changes are needed.
🏁 Script executed:
#!/bin/bash # Description: Search for GC-related changes in the codebase. # Test: Search for ICQ and unbonding record changes. Expect: Changes in GC logic. rg -A 5 'ICQ|unbonding.*record'Length of output: 35484
No state migration required – NoOpHandler is appropriate for this upgrade
After reviewing the GC-related changes in the repository, we confirmed that:
- Garbage collection for unbonding records is handled via runtime logic and, where necessary, through a dedicated upgrade (see the deletion logic in app/upgrades/v1_7.go).
- The changes for stale ICQ records follow similar patterns without requiring an explicit state migration during the V010707 upgrade.
Thus, using the NoOpHandler in app/upgrades/upgrades.go for V010707 is consistent with the overall implementation and does not introduce any issues.
app/upgrades/types.go (1)
57-57: LGTM!The new constant follows the correct version naming convention and is appropriately placed in the mainnet upgrades section.
1 participant
1. Summary
Release v1.7.7
Summary by CodeRabbit
Chores
Bug Fixes
New Features
Tests