build/ci: Update Go to v1.24, golangci-lint to v1.64.5 and fix issues #1804

thrasher- · 2025-02-14T03:48:02Z

PR Description

Bumps Go to v1.24, golangci-lint to v1.64.5 and fixes the issues it found.

Summary of changes:

Linter complained for the benchmark for i := 0; i < b.N; i++ loops and suggested to use a range as is standard now, however I opted instead to use b.Loop() which is new in Go1.24.
assertify any things which were touched.
Resolved several integer overflow issues, with an attempt to unify all var usage across the codebase and reduce type casting where possible.
kline.CalculateCandleDateRanges had a potential divide by 0 panic bug if limit was set to 0.
Config AES CFB has been switched to AES GCM as the prior one has been deprecated.
Removed HitBTC v1 authenticated endpoints (API on version 3)

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How has this been tested

go test ./... -race
golangci-lint run

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally and on Github Actions with my changes
Any dependent changes have been merged and published in downstream modules

Copilot

Copilot reviewed 90 out of 105 changed files in this pull request and generated 1 comment.

Files not reviewed (15)

.github/workflows/arm64.Dockerfile: Language not supported
Dockerfile: Language not supported
Makefile: Language not supported
backtester/btrpc/btrpc.swagger.json: Language not supported
cmd/gctcli/data_history.go: Evaluated as low risk
backtester/data/kline/kline.go: Evaluated as low risk
backtester/btrpc/btrpc.proto: Evaluated as low risk
backtester/engine/setup.go: Evaluated as low risk
cmd/gen_sqlboiler_config/main.go: Evaluated as low risk
.github/workflows/linter.yml: Evaluated as low risk
backtester/engine/grpcserver_test.go: Evaluated as low risk
.github/workflows/proto-lint.yml: Evaluated as low risk
backtester/btrpc/btrpc_grpc.pb.go: Evaluated as low risk
backtester/engine/grpcserver.go: Evaluated as low risk
.github/workflows/tests.yml: Evaluated as low risk

Comments suppressed due to low confidence (2)

backtester/config/strategyconfigbuilder/main.go:432

Ensure that the port value is correctly handled as uint32 throughout the codebase to avoid potential overflow issues.

port, err = strconv.ParseUint(input, 10, 32)

backtester/config/strategyconfigbuilder/main.go:437

[nitpick] The comment //nolint:gosec // No overflow risk needs to be justified better. Ensure that there is indeed no overflow risk.

cfg.DataSettings.DatabaseData.Config.Port = uint32(port) //nolint:gosec // No overflow risk

Copilot · 2025-02-14T05:38:16Z

backtester/btcli/commands.go

@@ -118,16 +119,19 @@ func executeStrategyFromFile(c *cli.Context) error {
 		}
 	}

-	var intervalOverride uint64
+	var intervalOverride int64


Add a check to ensure intervalOverride is not negative before converting it to time.Duration.

shazbert

Good stuff! nothing major I could see.

backtester/btrpc/btrpc.proto

config/config_encryption.go

exchanges/binanceus/type_convert.go

exchanges/bithumb/bithumb.go

exchanges/hitbtc/hitbtc.go

exchanges/kline/kline.go

shazbert

tACK tested old config encryption to this from master 🚀

gbjk

Looks good 👍

Only nits, and a few pedantic suggestions.

cmd/gctcli/commands.go

common/convert/convert.go

config/config_encryption.go

config/config_encryption_test.go

exchanges/bithumb/bithumb.go

exchanges/kline/kline.go

gbjk

Looks good to me 👍

shazbert

One suggestion changes look good.

exchanges/kline/kline.go

gloriousCode

That was a slog. Only thing that really stood out was usetesting being commented out

.golangci.yml

exchanges/hitbtc/hitbtc.go

exchanges/poloniex/poloniex.go

shazbert

tACK new changes. 🚀

gloriousCode

tACK!

gbjk

LGTM 🎉

gbjk · 2025-03-05T03:20:56Z

config/config_encryption.go

+	totalLen := len(encryptionPrefix) + len(c.storedSalt) + len(encryptionVersionPrefix) + versionSize + len(ciphertext)
+	appendedFile := make([]byte, totalLen)


I'm easy, but arguably:

Suggested change

totalLen := len(encryptionPrefix) + len(c.storedSalt) + len(encryptionVersionPrefix) + versionSize + len(ciphertext)

appendedFile := make([]byte, totalLen)

appendedFile := make([]byte, len(encryptionPrefix) + len(c.storedSalt) + len(encryptionVersionPrefix) + versionSize + len(ciphertext))

For me, it's very implicit that the maths there amounts to "total length"

gbjk · 2025-03-05T03:23:24Z

engine/engine_test.go

+			assert.NotEmpty(t, defaultCfg.Name)
+			assert.True(t, defaultCfg.Enabled)


I think that when you're not testing the overall thing, you have to add context for the error.
That is: require.NotEmpty(t, defaultCfg) is fine, because it's about the unit test's target func main return value.

Suggested change

assert.NotEmpty(t, defaultCfg.Name)

assert.True(t, defaultCfg.Enabled)

assert.NotEmpty(t, defaultCfg.Name, "Name should not be empty")

assert.True(t, defaultCfg.Enabled, "Enabled should have correct value")

thrasher- · 2025-03-06T01:17:03Z

This PR is pending release of sonic's next release: bytedance/sonic#741 (comment) which should land next week

gbjk

LGTM.

Only Nits.

backtester/eventhandlers/strategies/strategies.go

gbjk · 2025-03-08T06:07:39Z

backtester/eventhandlers/strategies/strategies_test.go

-		t.Errorf("received '%v' expected '%v'", resp, nil)
-	}
+	assert.ErrorIs(t, err, common.ErrNilPointer)
+	assert.Nil(t, resp)


🦟 Don't think we should need to check what resp is when err is not nil 🙂

Suggested change

assert.Nil(t, resp)

gbjk · 2025-03-08T06:08:55Z

backtester/eventhandlers/strategies/strategies_test.go

+	h = new(dollarcostaverage.Strategy)
+	resp, err = createNewStrategy(dollarcostaverage.Name, true, h)
+	assert.NoError(t, err, "createNewStrategy should not error")
+	assert.NotNil(t, resp)


🦟 Nit:

Suggested change

assert.NotNil(t, resp)

assert.NotSame(t, h, resp, "createNewStrategy must return a new pointer")

shazbert

Just something I noticed on new changes. Thanks!

shazbert · 2025-03-10T03:33:07Z

engine/engine_test.go

+				assert.Positive(t, defaultCfg.WebsocketResponseCheckTimeout, 0)
+				assert.Positive(t, defaultCfg.WebsocketResponseMaxLimit, 0)
+				assert.Positive(t, defaultCfg.WebsocketTrafficTimeout, 0)


Suggested change

assert.Positive(t, defaultCfg.WebsocketResponseCheckTimeout, 0)

assert.Positive(t, defaultCfg.WebsocketResponseMaxLimit, 0)

assert.Positive(t, defaultCfg.WebsocketTrafficTimeout, 0)

assert.Positive(t, defaultCfg.WebsocketResponseCheckTimeout)

assert.Positive(t, defaultCfg.WebsocketResponseMaxLimit)

assert.Positive(t, defaultCfg.WebsocketTrafficTimeout)

…1 and golangci-lint v1.64.6 Also addresses nits

gbjk

LGTM 🎉

shazbert

Lovely, thanks!

thrasher- self-assigned this Feb 14, 2025

thrasher- added the review me This pull request is ready for review label Feb 14, 2025

thrasher- requested review from shazbert, Copilot and gloriousCode February 14, 2025 05:35

thrasher- linked an issue Feb 14, 2025 that may be closed by this pull request

build/ci: Upgrade to Go 1.24 #1801

Closed

Copilot AI reviewed Feb 14, 2025

View reviewed changes

shazbert requested changes Feb 17, 2025

View reviewed changes

thrasher- force-pushed the go124_buildci branch 2 times, most recently from cdba415 to 27b8e73 Compare February 17, 2025 04:52

shazbert approved these changes Feb 17, 2025

View reviewed changes

shazbert added the szrc shazbert review complete label Feb 18, 2025

thrasher- force-pushed the go124_buildci branch from 27b8e73 to 311c8a8 Compare February 19, 2025 23:44

thrasher- requested a review from gbjk February 24, 2025 03:03

gbjk approved these changes Feb 27, 2025

View reviewed changes

thrasher- mentioned this pull request Feb 28, 2025

SQLBoiler: Change SQL ORM #1818

Open

thrasher- force-pushed the go124_buildci branch 2 times, most recently from 9c98fb6 to dee6f63 Compare February 28, 2025 10:50

gbjk approved these changes Mar 2, 2025

View reviewed changes

shazbert requested changes Mar 2, 2025

View reviewed changes

exchanges/kline/kline.go Show resolved Hide resolved

gloriousCode requested changes Mar 4, 2025

View reviewed changes

.golangci.yml Show resolved Hide resolved

exchanges/hitbtc/hitbtc.go Show resolved Hide resolved

exchanges/poloniex/poloniex.go Outdated Show resolved Hide resolved

shazbert approved these changes Mar 4, 2025

View reviewed changes

gloriousCode approved these changes Mar 4, 2025

View reviewed changes

thrasher- force-pushed the go124_buildci branch from 6360921 to 46aafb6 Compare March 4, 2025 06:33

gbjk approved these changes Mar 5, 2025

View reviewed changes

thrasher- mentioned this pull request Mar 5, 2025

modernise: Run new gopls modernise tool against the codebase #1826

Open

11 tasks

gloriousCode added the gcrc GloriousCode Review Complete label Mar 5, 2025

gbjk added nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged and removed nomerge requires dependency This pull request is dependent on another, so it can't be merged until the dependent one is merged labels Mar 7, 2025

gbjk approved these changes Mar 8, 2025

View reviewed changes

shazbert requested changes Mar 10, 2025

View reviewed changes

thrasher- added 7 commits March 10, 2025 15:02

build/ci: Update Go to v1.24, golangci-lint to v1.64.5 and fix issues

7471968

Address shazbert's nitters

dbc07d0

linter/config: Fix new linter issue and use versionSize const

3c5c482

Address gk's nitters and fix additional linter issue after rebase

71f2d1e

Address glorious nits

f5590bc

staticcheck: Fix additional linter issues after upgrading to Go 1.24.…

11d360a

…1 and golangci-lint v1.64.6 Also addresses nits

Improve testing, assertify usage and use common.ErrParsingWSField

39e5479

thrasher- force-pushed the go124_buildci branch from 070e09e to 39e5479 Compare March 10, 2025 04:12

gbjk approved these changes Mar 10, 2025

View reviewed changes

TestCreateNewStrategy: Replace must > should wording

2229703

gbjk approved these changes Mar 10, 2025

View reviewed changes

shazbert approved these changes Mar 10, 2025

View reviewed changes

thrasher- merged commit d64d56f into thrasher-corp:master Mar 10, 2025
5 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build/ci: Update Go to v1.24, golangci-lint to v1.64.5 and fix issues #1804

build/ci: Update Go to v1.24, golangci-lint to v1.64.5 and fix issues #1804

thrasher- commented Feb 14, 2025 •

edited by gloriousCode

Loading

Copilot AI left a comment

Copilot AI Feb 14, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

shazbert left a comment

shazbert left a comment

gbjk left a comment

gbjk left a comment

shazbert left a comment

gloriousCode left a comment

shazbert left a comment

gloriousCode left a comment

gbjk left a comment

gbjk Mar 5, 2025

gbjk Mar 5, 2025

thrasher- commented Mar 6, 2025

gbjk left a comment

gbjk Mar 8, 2025

gbjk Mar 8, 2025

shazbert left a comment

shazbert Mar 10, 2025

gbjk left a comment

shazbert left a comment

		totalLen := len(encryptionPrefix) + len(c.storedSalt) + len(encryptionVersionPrefix) + versionSize + len(ciphertext)
		appendedFile := make([]byte, totalLen)

	totalLen := len(encryptionPrefix) + len(c.storedSalt) + len(encryptionVersionPrefix) + versionSize + len(ciphertext)
	appendedFile := make([]byte, totalLen)
	appendedFile := make([]byte, len(encryptionPrefix) + len(c.storedSalt) + len(encryptionVersionPrefix) + versionSize + len(ciphertext))

		assert.NotEmpty(t, defaultCfg.Name)
		assert.True(t, defaultCfg.Enabled)

	assert.NotNil(t, resp)
	assert.NotSame(t, h, resp, "createNewStrategy must return a new pointer")

build/ci: Update Go to v1.24, golangci-lint to v1.64.5 and fix issues #1804

build/ci: Update Go to v1.24, golangci-lint to v1.64.5 and fix issues #1804

Conversation

thrasher- commented Feb 14, 2025 • edited by gloriousCode Loading

PR Description

Type of change

How has this been tested

Checklist

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Feb 14, 2025

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gbjk left a comment

Choose a reason for hiding this comment

gbjk left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

gloriousCode left a comment

Choose a reason for hiding this comment

gbjk left a comment

Choose a reason for hiding this comment

gbjk Mar 5, 2025

Choose a reason for hiding this comment

gbjk Mar 5, 2025

Choose a reason for hiding this comment

thrasher- commented Mar 6, 2025

gbjk left a comment

Choose a reason for hiding this comment

gbjk Mar 8, 2025

Choose a reason for hiding this comment

gbjk Mar 8, 2025

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

shazbert Mar 10, 2025

Choose a reason for hiding this comment

gbjk left a comment

Choose a reason for hiding this comment

shazbert left a comment

Choose a reason for hiding this comment

thrasher- commented Feb 14, 2025 •

edited by gloriousCode

Loading