-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
Bump go to v1.23 (from v1.21) #2132
Conversation
WalkthroughThe recent updates enhance the project by upgrading the Go language from version 1.21 to 1.23, which introduces new features and improvements. This change also includes updates to various GitHub Actions workflows for better Go version handling, improvements to linting configurations, and enhancements in documentation clarity. Additionally, several code adjustments improve readability, maintainability, and type safety across the application. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHubActions
participant GoModule
participant Linter
Developer->>GoModule: Update code and dependencies
Developer->>GitHubActions: Modify workflows for dynamic Go versioning
GitHubActions->>GoModule: Reference Go version from go.mod
GitHubActions->>Linter: Run lint checks with updated configurations
Linter-->>Developer: Provide feedback on code quality
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
…irect uses of the 'go' command with the $(GO) variable.
…tup does that automatically now.
…eed it are doing it on their own still anyway.
…3 (instead of 1.21).
…mp (since it's a pretty important change to note).
…tting rid of our copy of the Keys function.
…'s currently using v1.22 which causes make install to fail.
…expect it to anyway).
…eded golang version since the dockerfile might pick up on it.
…t the problem. Digging into their stuff, they don't yet have support for go v1.23, so we'll just have to wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (55)
- .changelog/unreleased/dependencies/2132-bump-go-to-1.23.md (1 hunks)
- .changelog/unreleased/summary.md (1 hunks)
- .github/workflows/docker.yml (1 hunks)
- .github/workflows/lint.yml (1 hunks)
- .github/workflows/proto.yml (1 hunks)
- .github/workflows/release.yml (4 hunks)
- .github/workflows/sims.yml (9 hunks)
- .github/workflows/test.yml (3 hunks)
- .golangci.yml (2 hunks)
- Makefile (5 hunks)
- README.md (1 hunks)
- app/prefix.go (2 hunks)
- app/prefix_test/prefix_test.go (2 hunks)
- cmd/provenanced/cmd/tree.go (1 hunks)
- contrib/devtools/Makefile (1 hunks)
- docker/blockchain/Dockerfile (1 hunks)
- docs/Building.md (1 hunks)
- go.mod (1 hunks)
- internal/antewrapper/min_gas_prices_decorator.go (1 hunks)
- internal/rand/int.go (1 hunks)
- internal/rand/int_test.go (2 hunks)
- networks/dev/blockchain-dev/Dockerfile (2 hunks)
- networks/ibc/blockchain-ibc/Dockerfile (2 hunks)
- networks/ibc/blockchain-relayer/Dockerfile (1 hunks)
- networks/local/blockchain-local/Dockerfile (2 hunks)
- testutil/sims.go (3 hunks)
- x/attribute/client/cli/tx.go (1 hunks)
- x/attribute/keeper/query_server.go (4 hunks)
- x/attribute/simulation/operations.go (3 hunks)
- x/exchange/client/cli/flags.go (2 hunks)
- x/exchange/client/cli/tx_test.go (4 hunks)
- x/exchange/keeper/msg_server.go (1 hunks)
- x/exchange/keeper/params.go (2 hunks)
- x/exchange/market_test.go (4 hunks)
- x/hold/keeper/keeper_test.go (3 hunks)
- x/marker/client/cli/tx.go (1 hunks)
- x/marker/keeper/msg_server.go (3 hunks)
- x/marker/keeper/params.go (1 hunks)
- x/marker/keeper/query_server.go (2 hunks)
- x/marker/simulation/genesis.go (1 hunks)
- x/marker/simulation/operations.go (6 hunks)
- x/marker/types/msgs.go (1 hunks)
- x/marker/types/si.go (1 hunks)
- x/metadata/keeper/msg_server.go (2 hunks)
- x/metadata/keeper/oslocatorparams.go (1 hunks)
- x/metadata/keeper/query_server.go (1 hunks)
- x/metadata/types/scope.go (2 hunks)
- x/msgfees/keeper/keeper.go (3 hunks)
- x/msgfees/types/msgs.go (1 hunks)
- x/name/client/cli/tx.go (1 hunks)
- x/name/keeper/keeper.go (1 hunks)
- x/name/simulation/genesis.go (1 hunks)
- x/oracle/simulation/genesis.go (1 hunks)
- x/trigger/keeper/event_detector.go (1 hunks)
- x/trigger/simulation/genesis.go (4 hunks)
Files skipped from review due to trivial changes (24)
- .changelog/unreleased/dependencies/2132-bump-go-to-1.23.md
- .github/workflows/docker.yml
- .github/workflows/proto.yml
- README.md
- cmd/provenanced/cmd/tree.go
- docs/Building.md
- internal/rand/int.go
- internal/rand/int_test.go
- networks/local/blockchain-local/Dockerfile
- x/attribute/keeper/query_server.go
- x/attribute/simulation/operations.go
- x/exchange/client/cli/flags.go
- x/exchange/keeper/params.go
- x/marker/client/cli/tx.go
- x/marker/keeper/params.go
- x/marker/simulation/genesis.go
- x/marker/types/si.go
- x/metadata/keeper/oslocatorparams.go
- x/metadata/keeper/query_server.go
- x/metadata/types/scope.go
- x/msgfees/types/msgs.go
- x/name/client/cli/tx.go
- x/name/simulation/genesis.go
- x/oracle/simulation/genesis.go
Additional context used
GitHub Check: docker
docker/blockchain/Dockerfile
[warning] 1-1: The 'as' keyword should match the case of the 'from' keyword
FromAsCasing: 'as' and 'FROM' keywords' casing do not match
More info: https://docs.docker.com/go/dockerfile/rule/from-as-casing/
Additional comments not posted (48)
.changelog/unreleased/summary.md (1)
1-2
: Documentation updates are clear and concise.The changelog summary effectively communicates the new requirements for building and linting.
networks/ibc/blockchain-relayer/Dockerfile (1)
2-2
: Base image update aligns with PR objectives.The Dockerfile now uses Go 1.23, which is consistent with the project's updated requirements.
.github/workflows/lint.yml (2)
33-33
: Dynamic Go version management improves adaptability.The workflow now references
go.mod
for the Go version, enhancing flexibility.
38-38
: Updated GolangCI Lint action version.The workflow uses
golangci-lint
version 1.60, ensuring the latest linting capabilities.app/prefix.go (1)
10-11
: Explicit typing of constants enhances type safety.The explicit declaration of
CoinTypeMainNet
andCoinTypeTestNet
asuint32
improves type safety and clarity. The updates in theSetConfig
function are consistent with these changes.Also applies to: 42-42
networks/ibc/blockchain-ibc/Dockerfile (1)
6-6
: Go version update in Dockerfile approved.The update to
golang:1.23-bullseye
for both x86_64 and ARM architectures is consistent with the PR's objective and is correctly implemented.Also applies to: 55-55
networks/dev/blockchain-dev/Dockerfile (1)
5-5
: Verify compatibility with Go 1.23.The Dockerfile has been updated to use Go 1.23. Ensure that all dependencies and code are compatible with this version to avoid runtime issues.
Run the following script to search for any Go version-specific dependencies or code that might need attention:
Also applies to: 51-51
internal/antewrapper/min_gas_prices_decorator.go (1)
64-64
: Improved precision in gas limit conversion.The change to use
LegacyNewDecFromBigInt
enhances precision when converting the gas limit to a decimal representation. This is a beneficial update.contrib/devtools/Makefile (1)
83-83
: Updated GolangCI Lint to v1.60.2.The update to GolangCI Lint version v1.60.2 ensures that the latest linting features and fixes are applied. This is a positive change.
x/trigger/keeper/event_detector.go (1)
56-57
: Improved type consistency in block height comparisons.The use of
uint64
forcurHeight
ensures type consistency in block height comparisons, reducing the risk of type-related issues.Also applies to: 61-62
.golangci.yml (1)
12-12
: Updated linter configuration for improved code quality.The addition of
copyloopvar
and removal ofexportloopref
reflect a refined linting strategy. The exclusion of(fmt.State).Write
fromerrcheck
suggests specific error handling for this function.Also applies to: 153-155
app/prefix_test/prefix_test.go (1)
40-40
: Enhanced type safety and test clarity.Changing
expCoinType
touint32
ensures type alignment with the application logic. The improved error messages provide clearer test failure information.Also applies to: 154-155
x/marker/keeper/query_server.go (2)
41-41
: Simplified error handling inAllMarkers
.The change from
status.Errorf
tostatus.Error
simplifies error creation but removes formatting capabilities. Ensure that the error message clarity is not compromised.
65-65
: Simplified error handling inMarker
.The change from
status.Errorf
tostatus.Error
simplifies error creation but removes formatting capabilities. Ensure that the error message clarity is not compromised..github/workflows/sims.yml (4)
66-66
: Usego-version-file
for dynamic version management.The change to use
go-version-file: 'go.mod'
allows the workflow to derive the Go version directly from the module file. This simplifies version management.
95-95
: Usego-version-file
for dynamic version management.The change to use
go-version-file: 'go.mod'
allows the workflow to derive the Go version directly from the module file. This simplifies version management.
147-147
: Usego-version-file
for dynamic version management.The change to use
go-version-file: 'go.mod'
allows the workflow to derive the Go version directly from the module file. This simplifies version management.
184-184
: Usego-version-file
for dynamic version management.The change to use
go-version-file: 'go.mod'
allows the workflow to derive the Go version directly from the module file. This simplifies version management.x/trigger/simulation/genesis.go (3)
139-141
: Improved parameter naming inrandomTriggerAndQueueIDs
.The parameter
max
has been renamed tomaxCount
, enhancing code readability by clearly indicating its purpose.
145-149
: Improved parameter naming and logic inrandomTriggerIDs
.The parameter
max
has been renamed tomaxCount
, and the logic now usesmaxCount
for clarity. The panic message has been updated to reflect this change.
206-208
: Added validation fortriggerID
inRandomizedGenState
.A new check ensures that
triggerID
does not exceedmath.MaxInt
, preventing potential runtime errors. This enhances the robustness of the code..github/workflows/test.yml (1)
106-106
: Approved: Usego.mod
for Go version management.Switching to
go-version-file: 'go.mod'
ensures that the workflow always uses the correct Go version specified by the project, enhancing consistency and maintainability.Also applies to: 193-193
.github/workflows/release.yml (1)
70-70
: Approved: Usego.mod
for Go version management.Switching to
go-version-file: 'go.mod'
ensures that the workflow always uses the correct Go version specified by the project, enhancing consistency and maintainability.Also applies to: 95-95, 165-165
x/msgfees/keeper/keeper.go (3)
11-11
: Approved: Importsdkmath
for improved precision.The addition of
sdkmath
enhances the precision of mathematical operations, which is beneficial for handling coin amounts accurately.
185-187
: Approved: Usesdkmath
fornhashPerMil
calculation.The change to use
sdkmath.NewIntFromUint64
andMul
improves the precision and robustness of theConvertDenomToHash
function.
313-316
: Approved: SimplifyDetermineBips
logic.The refined logic in
DetermineBips
enhances readability by directly checkingbips64
against the upper limit before assigningbips
.go.mod (1)
3-3
: Go version update approved.The Go version has been successfully updated to 1.23. Ensure compatibility with all dependencies.
x/attribute/client/cli/tx.go (1)
342-343
: Type conversion refactor approved.The explicit type conversion to
uint32
improves readability and clarity.x/name/keeper/keeper.go (1)
266-271
: Type change for segment length approved.Changing
segLen
toint
ensures proper type alignment for comparisons. Verify the impact on related logic, especially where negative values or larger integers might be involved.Run the following script to verify the impact on related logic:
Verification successful
Type change for
segLen
verified. The change toint
aligns with the logic inkeeper.go
and ensures consistent type alignment for comparisons. No issues found with this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of changing segLen to int in related logic. # Test: Search for usage of Normalize function. Expect: No issues with type alignment. rg --type go -A 5 $'Normalize'Length of output: 52377
Script:
#!/bin/bash # Search for usage of segLen and related logic in keeper.go to verify the impact of changing segLen to int. rg --type go 'segLen' -A 5 -B 5Length of output: 899
testutil/sims.go (2)
Line range hint
11-20
: LGTM! Enhanced logging flexibility.The addition of a format specifier in
LogOperationMsg
improves logging flexibility.
41-43
: Verify usage ofGenerateTestingAccounts
.The new function
GenerateTestingAccounts
is correctly implemented. Ensure that it is used appropriately in the codebase.Run the following script to verify the function usage:
Verification successful
Function
GenerateTestingAccounts
is correctly integrated and used.The function
GenerateTestingAccounts
is appropriately used across multiple test files, confirming its integration into the testing framework.
- Files using
GenerateTestingAccounts
:
x/trigger/simulation/operations_test.go
x/oracle/simulation/operations_test.go
x/marker/simulation/operations_test.go
testutil/sims.go
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of `GenerateTestingAccounts`. # Test: Search for the function usage. Expect: Occurrences of the new function. rg --type go -A 5 $'GenerateTestingAccounts'Length of output: 4527
x/exchange/keeper/msg_server.go (1)
331-333
: LGTM! Simplified error handling.The change from
Wrapf
toWrap
simplifies error handling without affecting functionality.Makefile (3)
37-39
: LGTM! Updated Go version to 1.23.The supported Go version is correctly updated to 1.23.
131-131
: LGTM! Enhanced installation process.The addition of Go version validation to the
install
target improves robustness.
Line range hint
485-495
: LGTM! Improved command consistency.The use of the
$(GO)
variable enhances consistency in Go module management commands.x/marker/types/msgs.go (1)
427-429
: Simplification ofAccessList
validation is appropriate.The removal of the
nil
check forAccessList
is a good simplification, assuming thatAccessList
is always initialized. This enhances code readability without compromising correctness.x/marker/simulation/operations.go (5)
112-115
: Introduction ofrandMarkerType
improves readability.The use of a dedicated function
randMarkerType
to determine the marker type enhances code readability and maintainability. This encapsulation is a good practice.
202-202
: Use ofrandMarkerType
inSimulateMsgAddFinalizeActivateMarker
.The refactoring to use
randMarkerType
here improves consistency and readability across the codebase.
236-237
: Clarification of variable names enhances understanding.Renaming variables like
max
tomaxLen
andmin
tominLen
inrandomUnrestrictedDenom
function improves clarity and aligns the names with their purpose.
533-538
: Renaming parameter inrandomInt63
improves clarity.Changing the parameter name from
max
tomaxVal
clarifies its intent, enhancing code readability.
540-541
: Addition ofrandMarkerType
function is well-implemented.The new function
randMarkerType
encapsulates the logic for determining marker types, improving code organization and readability.x/marker/keeper/msg_server.go (2)
117-118
: Improved precision inAddMarker
withsdkmath.NewIntFromUint64
.The change to use
sdkmath.NewIntFromUint64
forUsdMills
enhances precision and correctness in handling monetary values. This is a positive update for financial calculations.
520-521
: Enhanced precision inAddFinalizeActivateMarker
.Using
sdkmath.NewIntFromUint64
forUsdMills
ensures better precision in financial calculations, which is a crucial improvement.x/metadata/keeper/msg_server.go (1)
49-50
: LGTM! Improved type safety forUsdMills
.The change to use
sdkmath.NewIntFromUint64
andtypes.NewNetAssetValue
enhances type safety and robustness in handlingUsdMills
.x/hold/keeper/keeper_test.go (1)
5-6
: LGTM! Use of standard library functions.The transition to using
maps.Keys
andslices.Collect
from the standard library enhances readability and maintainability.x/exchange/client/cli/tx_test.go (1)
6-7
: LGTM! Use of standard library functions.The transition to using
maps.Keys
andslices.Collect
from the standard library enhances readability and maintainability.x/exchange/market_test.go (2)
3157-3157
: Great use of Go's standard library!The use of
maps.Keys
andslices.Collect
enhances readability and performance by leveraging Go's built-in capabilities.
3334-3334
: Consistent use of Go's standard library!The use of
maps.Keys
andslices.Collect
is consistent with best practices and improves code clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lots of little clean up in here ... I like the removal of a bunch of fmt.Sprintf calls
Description
This PR:
go
needed/used to v1.23 (from v1.21).golangci-lint
needed/used to v1.60.2 (from v1.54.2).internal/collections/maps.go
since it's only function (Keys
) is now part of themaps
built-in library.You can update your local
golangci-lint
usingmake golangci-lint-update
, or install it usingmake golangci-lint
. Those targets aren't new, but have been updated to use the new version.As of Wed, Aug 21, the
heighliner
job (in theDocker Build and Push
workflow) is failing because it doesn't yet support go v1.23. Once they do, that job should fix itself. I created issue #270 with them to add support for go v1.23.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/
).godoc
comments..changelog/unreleased
(see Adding Changes).Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style
Chores