fix: fix the query for max withdraw #2638
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested labels
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 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/leverage/keeper/grpc_query.go(1 hunks)
🔇 Additional comments (1)
x/leverage/keeper/grpc_query.go (1)
Line range hint 303-359: LGTM on error handling
The error handling in MaxWithdraw is comprehensive and follows good practices:
- Proper input validation
- Clear distinction between oracle and non-oracle errors
- Appropriate handling of edge cases
| if !t.Blacklist { | ||
| denoms = append(denoms, t.BaseDenom) | ||
| } | ||
| denoms = append(denoms, t.BaseDenom) |
There was a problem hiding this comment.
Inconsistent blacklist filtering between MaxWithdraw and MaxBorrow
There's an inconsistency in how blacklisted tokens are handled:
MaxWithdrawnow includes all registered tokensMaxBorrowstill filters out blacklisted tokens
This divergence in behavior needs clarification:
- If blacklisted tokens should be excluded from borrowing but allowed for withdrawal, this is correct but should be documented.
- If this is unintentional, one of these methods needs to be updated for consistency.
Would you like me to help document this behavioral difference or align the implementations?
Also applies to: 391-397
💡 Codebase verification
Based on the search results, I can now generate the final response:
Blacklist filtering should be maintained in MaxWithdraw
The removal of blacklist filtering in MaxWithdraw appears to be incorrect based on the codebase analysis:
- Blacklisted tokens are consistently filtered out across the codebase:
- In
MaxBorrowquery, blacklisted tokens are filtered (if !t.Blacklist) - Oracle hooks skip blacklisted tokens (
if token.Blacklist { return }) - Token validation requires
AssertNotBlacklisted() - The protocol documentation states blacklisted assets should be "treated as though its oracle price is zero, and thus ignored by calculations"
- In
The change introduces inconsistency with the rest of the codebase's treatment of blacklisted tokens and should maintain the blacklist filtering.
🔗 Analysis chain
Verify the removal of blacklist filtering
The change removes the blacklist filtering for tokens in MaxWithdraw, allowing all registered tokens to be considered. While this aligns with fixing the max withdraw query, we should verify if this is the intended behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if blacklist filtering is consistently applied across the codebase
# Search for blacklist usage in token filtering
rg -A 5 'Blacklist'
# Search for MaxWithdraw related tests
ast-grep --pattern 'func TestMaxWithdraw($$$)'
Length of output: 19089
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
.github/workflows/release-umee.yml (1)
Line range hint
1-50: PR scope mismatch: Go version updates vs. max withdraw query fixThis PR's title and description indicate it's meant to fix the query for max withdraw, but these changes are updating Go versions across workflows. This seems to be outside the scope of the original PR objective.
Consider:
- Splitting these Go version updates into a separate PR
- Updating the PR title and description if these changes are intentional
- Removing these changes if they were accidentally included
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/debug.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/release-umee.yml(1 hunks).github/workflows/simulations.yml(5 hunks).github/workflows/tests.yml(4 hunks)contrib/images/umee.e2e.dockerfile(2 hunks)contrib/images/umeed.dockerfile(2 hunks)
🔇 Additional comments (3)
.github/workflows/codeql-analysis.yml (1)
30-30:
Caution: Go 1.23 is not yet officially released
The workflow is being updated to use Go 1.23, which hasn't been officially released yet. This could potentially cause instability in the CodeQL security analysis process. Consider keeping the current stable version until Go 1.23 is officially released.
Let's verify the latest stable Go version:
.github/workflows/simulations.yml (1)
18-18: Verify Go 1.23 compatibility and stability
The workflow has been updated to use Go 1.23 across all jobs. Since this is a relatively new Go version, please ensure:
- All dependencies are compatible with Go 1.23
- The version is production-ready and stable
- The upgrade aligns with the project's Go version policy
Also applies to: 43-43, 68-68, 98-98, 127-127
.github/workflows/tests.yml (1)
18-18: Changes align with simulations.yml updates
The Go version updates are consistent with the changes in simulations.yml.
Also applies to: 55-55, 81-81, 113-113
| @@ -17,7 +17,7 @@ jobs: | |||
| steps: | |||
| - uses: actions/setup-go@v5 | |||
| with: | |||
| go-version: "1.22" | |||
| go-version: "1.23" | |||
There was a problem hiding this comment.
Invalid Go version specified
The specified Go version "1.23" is not yet released. The latest stable version is Go 1.22.x. Using an unreleased version will cause CI failures.
Apply this diff to fix the Go version:
- go-version: "1.23"
+ go-version: "1.22"📝 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-version: "1.23" | |
| go-version: "1.22" |
| @@ -1,7 +1,7 @@ | |||
| # Docker for e2e testing | |||
| # Creates dynamic binaries, by building from the latest version of umeed | |||
|
|
|||
| FROM golang:1.22-bookworm AS builder | |||
| FROM golang:1.23-bookworm AS builder | |||
There was a problem hiding this comment.
Invalid Go version in base image
The specified Go version "1.23" in the base image is not yet released. The latest stable version is Go 1.22.x.
Apply this diff to fix the base image:
-FROM golang:1.23-bookworm AS builder
+FROM golang:1.22-bookworm AS builder📝 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.
| FROM golang:1.23-bookworm AS builder | |
| FROM golang:1.22-bookworm AS builder |
| @@ -18,7 +18,7 @@ | |||
| RUN make install | |||
|
|
|||
| ## Prepare the final clear binary | |||
| FROM ubuntu:23.04 | |||
| FROM ubuntu:24.04 | |||
There was a problem hiding this comment.
Invalid Ubuntu version in base image
Ubuntu 24.04 is not yet released. Consider using Ubuntu 22.04 LTS for better stability and long-term support.
Apply this diff to fix the base image:
-FROM ubuntu:24.04
+FROM ubuntu:22.04📝 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.
| FROM ubuntu:24.04 | |
| FROM ubuntu:22.04 |
| @@ -14,7 +14,7 @@ RUN LEDGER_ENABLED=false BUILD_TAGS=badgerdb make install | |||
|
|
|||
| # Stage-2: copy binary and required artifacts to a fresh image | |||
| # we need to use debian compatible system. | |||
| FROM ubuntu:23.04 | |||
| FROM ubuntu:24.04 | |||
There was a problem hiding this comment.
Invalid Ubuntu version in base image
Ubuntu 24.04 is not yet released. Consider using Ubuntu 22.04 LTS for better stability and long-term support.
Apply this diff to fix the base image:
-FROM ubuntu:24.04
+FROM ubuntu:22.04📝 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.
| FROM ubuntu:24.04 | |
| FROM ubuntu:22.04 |
| @@ -1,7 +1,7 @@ | |||
| # Stage-1: build | |||
| # We use Debian Bullseye rather then Alpine because Alpine has problem building libwasmvm | |||
| # - requires to download libwasmvm_muslc from external source. Build with glibc is straightforward. | |||
| FROM golang:1.22-bookworm AS builder | |||
| FROM golang:1.23-bookworm AS builder | |||
There was a problem hiding this comment.
Invalid Go version in base image
The specified Go version "1.23" in the base image is not yet released. The latest stable version is Go 1.22.x.
Apply this diff to fix the base image:
-FROM golang:1.23-bookworm AS builder
+FROM golang:1.22-bookworm AS builder📝 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.
| FROM golang:1.23-bookworm AS builder | |
| FROM golang:1.22-bookworm AS builder |
| @@ -22,7 +22,7 @@ jobs: | |||
| - uses: actions/setup-go@v5 | |||
| if: env.GIT_DIFF | |||
| with: | |||
| go-version: "1.22" | |||
| go-version: "1.23" | |||
There was a problem hiding this comment.
💡 Codebase verification
Go 1.23 is not yet supported by golangci-lint
The golangci-lint tool currently uses Go 1.22.1 as specified in its go.mod file. Using Go 1.23 in the workflow could lead to compatibility issues since:
- The linter hasn't been tested or updated for Go 1.23
- New language features in Go 1.23 might not be properly analyzed
- This could result in unreliable linting results or workflow failures
Please update the Go version in the workflow to 1.22 to ensure compatibility with golangci-lint.
🔗 Analysis chain
Verify golangci-lint compatibility with Go 1.23
Using an unreleased Go version (1.23) could cause compatibility issues with golangci-lint. Additionally:
- The linter might not fully support Go 1.23 features yet
- This could lead to false positives or missed issues in the lint process
Let's check golangci-lint's Go compatibility:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check golangci-lint's supported Go versions
curl -s https://raw.githubusercontent.com/golangci/golangci-lint/master/go.mod | grep -A 1 "go "
Length of output: 390
| @@ -22,7 +22,7 @@ jobs: | |||
| fetch-depth: 0 | |||
| - uses: actions/setup-go@v5 | |||
| with: | |||
| go-version: "1.22" | |||
| go-version: "1.23" | |||
There was a problem hiding this comment.
Critical: Avoid using unreleased Go version for production releases
Using Go 1.23 (unreleased) for building release binaries poses significant risks:
- Potential instability in production builds
- Lack of security patches and updates for an unreleased version
- Possible compatibility issues with dependencies
Consider maintaining the current stable Go version for production releases.
There was a problem hiding this comment.
Can you check again, it is already released
https://tip.golang.org/doc/go1.23
There was a problem hiding this comment.
@gsk967, thank you for the clarification. You're absolutely right; Go 1.23 has been released. The update is appropriate.
(A curious rabbit tilts its head, ears perked up)
✏️ Learnings added
Learnt from: gsk967
PR: umee-network/umee#2638
File: .github/workflows/release-umee.yml:25-25
Timestamp: 2024-12-06T18:52:26.187Z
Learning: Go 1.23 has been officially released and is acceptable for production releases.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2638 +/- ##
===========================================
- Coverage 75.38% 62.00% -13.39%
===========================================
Files 100 275 +175
Lines 8025 19627 +11602
===========================================
+ Hits 6050 12170 +6120
- Misses 1589 6685 +5096
- Partials 386 772 +386
|
Description
closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit