smaller PR #659

faddat · 2025-04-25T02:52:45Z

NOTE: this is not the juicy PR, it just gets us moving.

Please, review this, it is easy. Then I can get to work on revive and gocritic and good lord, #654

chipshort

Thanks for creating this smaller PR. I put some comments inline.

chipshort · 2025-04-25T15:11:51Z

.golangci.yml

+    # Additional recommended linters
+    - gosec # Security checker
+    - misspell # Find commonly misspelled words
+    - bodyclose # Check HTTP response bodies are closed


This one seems a bit out of place. We don't handle HTTP requests here. Let's remove it.

eh, let's keep it in case we ever do. Its there to prevent mistakes sir. Now we know we pass that linter. There are many more to pass.

Sir, this is glue code between a blockchain sdk and a VM. It will never do any HTTP requests.

Pass the linter anyhow. The idea is to have e'm all (well realistically the right ones) working right. So I can remove it, but there's no reason to it eats like .1s or something and provides a layer of protection in the future.

Please remove. If I accept useless changes like this, the repo will bloat in no time.

internal/api/callbacks.go

cmd/demo/main.go

chipshort · 2025-04-25T15:49:40Z

cmd/demo/main.go

+	if filepath.IsAbs(cleanPath) || strings.Contains(cleanPath, "..") {
+		fmt.Println("Error: invalid file path")
+		exitCode = 1
+		return
+	}


Why go out of our way to reject absolute paths and paths in the parent directory?

slavishly satisfying the linter's high standards

Why is this a "high standard"? Are there possible problems with accepting absolute paths or paths inside the parent directory? Otherwise, this is needlessly restrictive.
Which linter causes this? Are there any explanations for this lint?

I think it's gosec. And the reasoning is validating file paths a bit insanely. They have a numbered system. I'll dig it up.

yeah, it's a security thing, suggest we keep it. It's basically keeping us vigilant here, I think that's okay

Please remove and mark as nolint. A lint like this makes sense in a webserver environment where you want people to only access relative paths inside a directory. This is a cli application where the path is supplied by the person who executes it.

internal/api/lib.go

internal/api/lib_test.go

chipshort · 2025-04-25T17:13:28Z

lib_libwasmvm.go

-		for i, m := range response.SubMessages() {
+		messages := response.SubMessages()
+		for i := 0; i < len(messages); i++ {


Why this change? VS Code even marks this as an issue for me and recommends to use range.

it's because the repo bounced between using versions of Go. Feel free to change it back or what have you. We should always use the latest release of Go, and that's currently 1.24.

When we DO NOT always use the latest release of Go, validators and chain teams compile with all kinds of different freaky go's and then the chain has a bad time due to changes in the go runtime.

so, ALWAYS use the latest GO

by FORCING best practices on downstream, we will improve code quality in actual operational settings significantly.

Validators and the like are free to be dumb by using any old version of Go.

We, as maintainers, aren't, and we should look out for the end users, who in this case are mainly validators.

I get your point about forcing best practices on downstream and I think there is some truth to it. I think it's about striking a balance between forcing best practices and keeping some wiggle room for users in case the best practice doesn't work for them or doesn't make sense in their case.
We've had problems with this on the Rust side before, where a Rust update enables some new wasm proposal, making all contracts compiled with that version fail to upload (because the VM doesn't fully support the proposal yet). If the Rust libraries followed your rule, that would be hell for contract devs.
On the topic of latest Go:

The Cosmos SDK also doesn't enforce this, so I don't see why we should do that. The way you describe it, the problem is a general one in the Cosmos ecosystem, so a solution should not only work for CosmWasm chains.

The docs of the go directive specifically say that it's supposed to be the "minimum version of Go required to compile packages in this module". That doesn't sound to me like the intention is to force downstream to use latest Go.

Do you have any examples of chains that had "a bad time due to changes in the go runtime"? The way you describe it as "freaky go's" sounds a bit manipulative to me. I'm sure you didn't mean it like that. Having some examples of that would make for a stronger case. Especially, since minor Go updates are supposed to be non-breaking.

Yeah, so there are countless examples of chains that had a bad time due to MIXED go runtimes, and ZERO examples of chains that had a bad time due to HAVING EXACTLY THE SAME RUNTIME.

So imo, that's the trick but there's a big added benefit:

by using the latest language runtime, we get all of the latest language features and fixes

...also they're not really being forced to do anything - anyone can compile whatever crazy config they want anyhow.

Does your vscode still flag this?

Yes.

for loop can be modernized using range over int modernize(rangeint)

It recommends:

for i := range messages {

And in fact, I would say this would be even nicer to get rid of the messages[i]:

for i, m := range messages {

Co-authored-by: Christoph Otter <chipshort@tutanota.com>

chipshort · 2025-05-02T08:34:09Z

internal/api/lib_test.go

-	tmpdir := t.TempDir()
+	tmpdir, err := os.MkdirTemp("", "wasmvm-testing")
+	require.NoError(t, err)
+	defer func() {
+		if err := os.RemoveAll(tmpdir); err != nil {
+			t.Errorf("failed to remove temp dir: %v", err)
+		}
+	}()


There are still lots of t.TempDir() removals here. Please undo

it's fair. I need to figure out why it does this though, and keep it ifxed.

chipshort · 2025-05-02T08:49:46Z

.golangci.yml

+    # Additional recommended linters
+    - gosec # Security checker
+    - misspell # Find commonly misspelled words
+    - bodyclose # Check HTTP response bodies are closed


Please remove. If I accept useless changes like this, the repo will bloat in no time.

chipshort · 2025-05-02T08:54:25Z

cmd/demo/main.go

+	if filepath.IsAbs(cleanPath) || strings.Contains(cleanPath, "..") {
+		fmt.Println("Error: invalid file path")
+		exitCode = 1
+		return
+	}


Please remove and mark as nolint. A lint like this makes sense in a webserver environment where you want people to only access relative paths inside a directory. This is a cli application where the path is supplied by the person who executes it.

chipshort · 2025-05-02T09:02:10Z

lib_libwasmvm.go

-		for i, m := range response.SubMessages() {
+		messages := response.SubMessages()
+		for i := 0; i < len(messages); i++ {


Yes.

for loop can be modernized using range over int modernize(rangeint)

It recommends:

for i := range messages {

And in fact, I would say this would be even nicer to get rid of the messages[i]:

for i, m := range messages {

faddat · 2025-05-04T10:28:04Z

@chipshort - gosec is literally the golang security linter and I do not think it is any sort of a good idea to remove it.

It checks for common security issues, and removing it kinda defeats the purpose of this exercise sir.

chipshort · 2025-05-05T08:10:01Z

@chipshort - gosec is literally the golang security linter and I do not think it is any sort of a good idea to remove it.

It checks for common security issues, and removing it kinda defeats the purpose of this exercise sir.

I didn't mean to remove the whole linter. Only the change I commented on and mark it as nolint.

faddat · 2025-05-06T02:59:35Z

chipshort · 2025-05-06T08:09:56Z

That's in reference to the line I am commenting on, namely the bodyclose linter, not gosec.

faddat added 25 commits December 22, 2024 20:03

enable additional linters

fdaf809

Merge branch 'faddat/testifylint' into faddat/bump-go

016154b

adjust golang version in circleci

71b2d1e

bump go...

f371399

remove dupl

cbde717

lint main.go

08ea4c4

lints...

7a812f5

complete

65039a5

Merge branch 'main' into faddat/bump-go

ada59f4

remove mutext lock in lib_test.go

a06cd0d

remove unused variable

1118f26

restore comments

d6c14e6

fix lint in .golangci.yml

d943802

Merge branch 'faddat/errcheck' into faddat/bump-go

f9e29a6

lint

ce052d6

Refactor test helper function for creating message binary

59fa5a4

revert unnecessary change

20aaea0

resolve merge conflict and reduce required go version

19d75a5

adjust loop to go 1.21

f74e259

Merge remote-tracking branch 'origin/main' into before-revive

1aa9993

all linters excecpt revive

66ff709

gocritic

e7e28b8

disable gocritic temporarily

703cc45

update linter for v2

2e9424b

really get it working for v2

71448a8

chipshort reviewed Apr 25, 2025

View reviewed changes

faddat and others added 4 commits April 27, 2025 14:53

Update internal/api/lib_test.go

6838413

Co-authored-by: Christoph Otter <chipshort@tutanota.com>

bring back useful comments

a574461

Merge remote-tracking branch 'origin/main' into faddat/smaller-pr

0e58174

use C.ErrnoVALUE_Success

c0d7ea2

faddat added 2 commits April 30, 2025 04:08

I cannot argue about this :)

ff1c56d

eliminate double declaration of CapitalizedResponse

c9bc165

chipshort reviewed May 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

smaller PR #659

smaller PR #659

faddat commented Apr 25, 2025 •

edited

Loading

chipshort left a comment

chipshort Apr 25, 2025

faddat Apr 27, 2025

chipshort Apr 28, 2025

faddat Apr 28, 2025

chipshort May 2, 2025

chipshort Apr 25, 2025

faddat Apr 27, 2025

chipshort Apr 28, 2025

faddat Apr 28, 2025

faddat Apr 29, 2025 •

edited

Loading

chipshort May 2, 2025

chipshort Apr 25, 2025

faddat Apr 27, 2025 •

edited

Loading

chipshort Apr 28, 2025

faddat Apr 28, 2025 •

edited

Loading

faddat Apr 29, 2025

chipshort May 2, 2025

chipshort May 2, 2025

faddat May 6, 2025

chipshort May 2, 2025

chipshort May 2, 2025

chipshort May 2, 2025

faddat commented May 4, 2025

chipshort commented May 5, 2025

faddat commented May 6, 2025

chipshort commented May 6, 2025

smaller PR #659

Are you sure you want to change the base?

smaller PR #659

Conversation

faddat commented Apr 25, 2025 • edited Loading

chipshort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faddat Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faddat Apr 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faddat Apr 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faddat commented May 4, 2025

chipshort commented May 5, 2025

faddat commented May 6, 2025

chipshort commented May 6, 2025

faddat commented Apr 25, 2025 •

edited

Loading

faddat Apr 29, 2025 •

edited

Loading

faddat Apr 27, 2025 •

edited

Loading

faddat Apr 28, 2025 •

edited

Loading