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.

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

bug(gnovm): incorrect type comparison #2386

Closed
moul opened this issue Jun 18, 2024 · 4 comments · Fixed by #1890
Closed

bug(gnovm): incorrect type comparison #2386

moul opened this issue Jun 18, 2024 · 4 comments · Fixed by #1890
Labels
🐞 bug Something isn't working

Comments

@moul
Copy link
Member

moul commented Jun 18, 2024

CleanShot 2024-06-18 at 16 59 30

https://go.dev/play/p/-y5gRjR9erA
https://play.gno.land/p/B9WLR_AsqYY


In GnoVM, type aliases with the same value are considered equal, while in Go, they are considered different without explicit casting. This leads to inconsistent behavior between Gno and Go.

Example

The following code returns false, false, false in Go, but true, false, false in Gno:

package main

import "fmt"

func check(v1, v2 interface{}) bool {
	return v1 == v2
}

func main() {
	type t1 int
	type t2 int
	v1 := t1(1)
	v2 := t2(1)
	v3 := t2(3)

	fmt.Println("v1, v2", v1, v2, check(v1, v2))
	fmt.Println("v1, v3", v1, v3, check(v1, v3))
	fmt.Println("v2, v3", v2, v3, check(v2, v3))
}
@thehowl
Copy link
Member

thehowl commented Jun 18, 2024

cc @ltzmaxwell is this fixed by #1426?

@deelawn
Copy link
Contributor

deelawn commented Jun 18, 2024

This is not fixed by #1426

@ltzmaxwell
Copy link
Contributor

this should be fixed in #1890.
similar context with #1376.

@moul moul added the 🐞 bug Something isn't working label Jun 19, 2024
zivkovicmilos pushed a commit that referenced this issue Jul 8, 2024
This PR introduces a new pattern for arbitrary govdao proposals using
Gno code using contexts. It involves wrapping the provided closure with
a system that configures a `context.Context` with a field indicating
that the execution occurs in the context of an `approvedByGovDao`
proposal. This opens the door to a new ACL system, not based on
`PrevRealm()`, but on a context-based "certification" system.

I believe this pattern makes sense to exist; however, here are some
drawbacks I can see:

1. `context.Context` is not yet needed (no goroutine support), and we
may eventually never need this package depending on how we implement
goroutines internally. (h/t @thehowl)
2. `context.Context` is used like an environment variable, which
introduces implicitness—something we usually try to avoid to make Gno a
very explicit language. Explicitness brings “simplicity” and
“verifiability.”
3. The usual Go idiomatic way of using `context` is to pass it as the
first argument. If this becomes more common, it will result in more
contracts having exposed functions that are not callable by `maketx
call` (though they can be called with `maketx run` or via an import).

Depends on #2379  
Related to #2386

---------

Signed-off-by: moul <[email protected]>
Co-authored-by: Antonio Navarro Perez <[email protected]>
gfanton pushed a commit to gfanton/gno that referenced this issue Jul 23, 2024
This PR introduces a new pattern for arbitrary govdao proposals using
Gno code using contexts. It involves wrapping the provided closure with
a system that configures a `context.Context` with a field indicating
that the execution occurs in the context of an `approvedByGovDao`
proposal. This opens the door to a new ACL system, not based on
`PrevRealm()`, but on a context-based "certification" system.

I believe this pattern makes sense to exist; however, here are some
drawbacks I can see:

1. `context.Context` is not yet needed (no goroutine support), and we
may eventually never need this package depending on how we implement
goroutines internally. (h/t @thehowl)
2. `context.Context` is used like an environment variable, which
introduces implicitness—something we usually try to avoid to make Gno a
very explicit language. Explicitness brings “simplicity” and
“verifiability.”
3. The usual Go idiomatic way of using `context` is to pass it as the
first argument. If this becomes more common, it will result in more
contracts having exposed functions that are not callable by `maketx
call` (though they can be called with `maketx run` or via an import).

Depends on gnolang#2379  
Related to gnolang#2386

---------

Signed-off-by: moul <[email protected]>
Co-authored-by: Antonio Navarro Perez <[email protected]>
@ltzmaxwell
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

4 participants