fix: iterate delegations continue instead of erroring #8732
Conversation
czarcas7ic
changed the title
czarcas7ic
requested review from
mattverse and
AlpinYukseloglu
as code owners
WalkthroughThe changes involve updates to the Changes
Warning Review ran into problems🔥 ProblemsError running LanguageTool: LanguageTool error: Unknown error 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
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.md (1)
Old CL swap APIs are still in use:
x/concentrated-liquidity/incentives_test.gox/concentrated-liquidity/swaps_tick_cross_test.gox/concentrated-liquidity/swapstrategy/zero_for_one.gox/concentrated-liquidity/swapstrategy/one_for_zero.gox/concentrated-liquidity/swaps_test.gox/concentrated-liquidity/position_test.gox/concentrated-liquidity/model/pool_test.gox/concentrated-liquidity/model/pool.gox/concentrated-liquidity/math/math.gox/concentrated-liquidity/math/math_test.gotests/e2e/e2e_cl_test.goapp/apptesting/concentrated_liquidity.gox/gamm/types/query.pb.gw.gox/gamm/types/query.pb.gox/gamm/keeper/grpc_query.gox/gamm/client/cli/query.gox/gamm/client/cli/cli_test.gocmd/osmosisd/cmd/stargate-query.go/.coderabbit_commands_1727393649383.sh🔗 Analysis chain
Line range hint
88-95: Bug fixes and API changes noted.Several bug fixes are mentioned, including improvements to error handling and mathematical operations. There are also API changes, particularly in the concentrated liquidity module. These changes may require updates to dependent code and should be carefully reviewed by developers using these APIs.
Please run the following corrected script to verify the usage of the old CL swap APIs:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any uses of the old CL swap APIs rg --type go 'CalcAmount0Delta|GetNextSqrtPriceFromAmount0InRoundingUp'Length of output: 64899
Script:
#!/bin/bash # Check for any uses of the old CL swap APIs rg --type go 'CalcAmount0Delta|GetNextSqrtPriceFromAmount0InRoundingUp'Length of output: 5807
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/superfluid/keeper/stake.go (1 hunks)
🔇 Additional comments (9)
CHANGELOG.md (9)
57-60: Minor improvements and bug fixes noted.The changes in this section appear to be mostly improvements and bug fixes. The state export fix for cwpool gauges is particularly important as it addresses a potential issue with the system's state management.
Line range hint
64-69: New features and improvements added.Several new features and improvements have been added, including new helper functions for SDK coins operations and additional queries in the concentrated liquidity module. These additions should enhance the functionality and efficiency of the system.
Line range hint
78-86: Performance improvements and new features added.This section introduces several performance improvements, particularly in mathematical operations and concentrated liquidity operations. New features like additional queries for concentrated liquidity are also added. These changes should enhance the overall efficiency and functionality of the system.
Line range hint
99-107: Major features and module updates introduced.This release introduces significant new features and modules, including the ProtoRev module for MEV capture, the Validator Set Preference module, and the Pool Manager module. These additions represent substantial enhancements to the system's functionality and may require careful integration and testing.
Line range hint
118-124: Dependency upgrades and configuration changes.The release includes upgrades to key dependencies like the Cosmos SDK, IBC, and CometBFT. There are also changes to default configurations, such as consensus parameters. These updates may require adjustments to node configurations and should be carefully reviewed by node operators.
Line range hint
128-133: Critical bug fixes and performance improvements.This release addresses important mainnet bugs related to accumulation sumtrees and concentrated liquidity handling. It also includes performance improvements for epoch computations. These changes are crucial for the stability and efficiency of the system.
Line range hint
150-5824: Historical changelog entries provide context.The remaining sections of the changelog document the development history of Osmosis, including various features, bug fixes, and improvements introduced in previous versions. While not directly relevant to the current release, this historical information is valuable for understanding the project's evolution and may be useful for developers working with older versions or tracking long-term changes.
Line range hint
137-146: API breaks and new features introduced.Several API breaks are mentioned, including changes to query responses and the introduction of new modules. New features like improved trade estimation and support for different chain IDs in E2E testing are also added. These changes may require updates to dependent code and should be carefully reviewed by developers.
#!/bin/bash # Check for any uses of the old API methods rg --type go 'EstimateSwapExactAmountIn|EstimateSwapExactAmountOut'
Line range hint
71-74: API breaks introduced.There are some API breaks mentioned here, particularly in the PoolModuleI CalculateSpotPrice API and some changes in the incentives module. These changes may require updates to dependent code.
| ctx.Logger().Error("intermediary account not found for lock id", "lockID", lock.UnderlyingLockId) | ||
| continue |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the implications of error logging vs. returning
The changes in the IterateDelegations function replace error returns with error logging. While this allows the function to continue processing delegations even when errors occur, it may hide important issues.
-
Pros:
- Increases robustness by continuing iteration despite errors.
- Provides more comprehensive delegation information.
-
Cons:
- May mask critical errors that should halt execution.
- Could lead to inconsistent state if errors are not properly handled downstream.
Consider the following:
- Are these errors expected to occur occasionally, or do they represent exceptional conditions?
- Could continuing after these errors lead to data inconsistencies?
- Is there a need to aggregate or report these errors for monitoring purposes?
You might want to consider a hybrid approach:
- Log the errors as currently implemented.
- Keep a count or collect details of the errors encountered.
- Return an aggregated error or error summary at the end of the function.
This would allow continued processing while still surfacing error information to callers.
func (k Keeper) IterateDelegations(context context.Context, delegator sdk.AccAddress, fn func(int64, stakingtypes.DelegationI) bool) error {
ctx := sdk.UnwrapSDKContext(context)
var errCount int
var errDetails []string
// ... (existing code)
for i, lock := range synthlocks {
// ... (existing code)
if !ok {
errCount++
errDetails = append(errDetails, fmt.Sprintf("intermediary account not found for lock id %d", lock.UnderlyingLockId))
ctx.Logger().Error("intermediary account not found for lock id", "lockID", lock.UnderlyingLockId)
continue
}
// ... (similar error handling for other error cases)
// if valid delegation has been found, increment delegation index
fn(index+int64(i), delegation)
}
if errCount > 0 {
return fmt.Errorf("encountered %d errors during iteration: %s", errCount, strings.Join(errDetails, "; "))
}
return nil
}Also applies to: 591-591, 597-597, 604-604, 611-611, 617-617, 623-623
Closes: #XXX
What is the purpose of the change
Frontports fix that was in v26.x to main