Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix the query for max withdraw #2638

Merged
merged 2 commits into from
Dec 8, 2024
Merged

fix: fix the query for max withdraw #2638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd79aa2
fix: fix the query for max withdraw
gsk967 Dec 6, 2024
6e30706
update the docker
gsk967 Dec 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/codeql-analysis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ jobs:
uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: "1.21"
go-version: "1.23"
# Initializes the CodeQL tools for scanning.
- name: Initialize CodeQL
uses: github/codeql-action/init@v3
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/debug.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
steps:
- uses: actions/setup-go@v5
with:
go-version: "1.22"
go-version: "1.23"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
go-version: "1.23"
go-version: "1.22"

cache: true

- uses: actions/checkout@v4
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:
- uses: actions/setup-go@v5
if: env.GIT_DIFF
with:
go-version: "1.22"
go-version: "1.23"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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:

  1. The linter might not fully support Go 1.23 features yet
  2. 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

cache: true

- name: golangci-lint main
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release-umee.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:
fetch-depth: 0
- uses: actions/setup-go@v5
with:
go-version: "1.22"
go-version: "1.23"
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical: Avoid using unreleased Go version for production releases

Using Go 1.23 (unreleased) for building release binaries poses significant risks:

  1. Potential instability in production builds
  2. Lack of security patches and updates for an unreleased version
  3. Possible compatibility issues with dependencies

Consider maintaining the current stable Go version for production releases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check again, it is already released
https://tip.golang.org/doc/go1.23

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

cache: true

- name: Set Env
Expand Down
10 changes: 5 additions & 5 deletions .github/workflows/simulations.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
steps:
- uses: actions/setup-go@v5
with:
go-version: "1.22"
go-version: "1.23"

- name: Install runsim
run: export GO111MODULE="on" && go install github.com/cosmos/tools/cmd/[email protected]
Expand All @@ -40,7 +40,7 @@ jobs:
- uses: actions/setup-go@v5
if: env.GIT_DIFF
with:
go-version: "1.22"
go-version: "1.23"
cache: true
- name: Test application non-determinism
if: env.GIT_DIFF
Expand All @@ -65,7 +65,7 @@ jobs:
- uses: actions/setup-go@v5
if: env.GIT_DIFF
with:
go-version: "1.22"
go-version: "1.23"
cache: true
- uses: actions/cache@v4
if: env.GIT_DIFF
Expand Down Expand Up @@ -95,7 +95,7 @@ jobs:
- uses: actions/setup-go@v5
if: env.GIT_DIFF
with:
go-version: "1.22"
go-version: "1.23"
cache: true
- uses: actions/cache@v4
if: env.GIT_DIFF
Expand Down Expand Up @@ -124,7 +124,7 @@ jobs:
- uses: actions/setup-go@v5
if: env.GIT_DIFF
with:
go-version: "1.22"
go-version: "1.23"
cache: true
- uses: actions/cache@v4
if: env.GIT_DIFF
Expand Down
8 changes: 4 additions & 4 deletions .github/workflows/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
steps:
- uses: actions/setup-go@v5
with:
go-version: "1.22"
go-version: "1.23"
- uses: actions/cache@v4
id: cache-go-tparse
with:
Expand Down Expand Up @@ -52,7 +52,7 @@ jobs:
if: steps.cache-binaries.outputs.cache-hit != 'true' && env.GIT_DIFF
uses: actions/setup-go@v5
with:
go-version: "1.22"
go-version: "1.23"
cache: true
env:
GOOS: ${{ matrix.targetos }}
Expand All @@ -78,7 +78,7 @@ jobs:
- uses: actions/setup-go@v5
if: env.GIT_DIFF
with:
go-version: "1.22"
go-version: "1.23"
cache: true
- name: Test and Create Coverage Report
if: env.GIT_DIFF
Expand Down Expand Up @@ -110,7 +110,7 @@ jobs:
- uses: actions/setup-go@v5
if: env.GIT_DIFF
with:
go-version: "1.22"
go-version: "1.23"
cache: true

- name: Test E2E
Expand Down
4 changes: 2 additions & 2 deletions contrib/images/umee.e2e.dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
FROM golang:1.23-bookworm AS builder
FROM golang:1.22-bookworm AS builder

ARG EXPERIMENTAL=true

## Download go module dependencies for umeed
Expand All @@ -18,7 +18,7 @@ RUN if [ "$EXPERIMENTAL" = "true" ] ; then echo "Installing experimental build";
RUN make install

## Prepare the final clear binary
FROM ubuntu:23.04
FROM ubuntu:24.04
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
FROM ubuntu:24.04
FROM ubuntu:22.04

EXPOSE 26656 26657 1317 9090 7171
ENTRYPOINT ["umeed", "start"]

Expand Down
4 changes: 2 additions & 2 deletions contrib/images/umeed.dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
FROM golang:1.23-bookworm AS builder
FROM golang:1.22-bookworm AS builder


WORKDIR /src/
# optimization: if go.sum didn't change, docker will use cached image
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
FROM ubuntu:24.04
FROM ubuntu:22.04

EXPOSE 26656 26657 1317 9090
# Run umeed by default, omit entrypoint to ease using container with CLI
CMD ["umeed"]
Expand Down
4 changes: 1 addition & 3 deletions x/leverage/keeper/grpc_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,7 @@ func (q Querier) MaxWithdraw(
} else {
// Denom not specified
for _, t := range q.GetAllRegisteredTokens(ctx) {
if !t.Blacklist {
denoms = append(denoms, t.BaseDenom)
}
denoms = append(denoms, t.BaseDenom)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Inconsistent blacklist filtering between MaxWithdraw and MaxBorrow

There's an inconsistency in how blacklisted tokens are handled:

  • MaxWithdraw now includes all registered tokens
  • MaxBorrow still filters out blacklisted tokens

This divergence in behavior needs clarification:

  1. If blacklisted tokens should be excluded from borrowing but allowed for withdrawal, this is correct but should be documented.
  2. 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 MaxBorrow query, 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"

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

}
}

Expand Down
Loading