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

Map package variable breaks the contract execution without exception #311

Closed
piux2 opened this issue Aug 4, 2022 · 1 comment · Fixed by #932
Closed

Map package variable breaks the contract execution without exception #311

piux2 opened this issue Aug 4, 2022 · 1 comment · Fixed by #932
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related

Comments

@piux2
Copy link
Contributor

piux2 commented Aug 4, 2022

package maptest

var m = make(map[string]string)

func Deposit() string{
  m["0xABC"] = "10ugnot"
  return "Deposited"
}

The maptest contract does not return anything when Deposit() is called a second time. The contract seems to have stopped executing before the return but no panic or exception.

@moul moul added the bug label Aug 9, 2022
@moul
Copy link
Member

moul commented Oct 20, 2022

Golang's team intentionally made it random, starting with Go 1, to make developers not rely on it.
See https://go.dev/blog/maps.

Protobuf also specifies that they have fewer guarantees and implements them as slices of structs on the wire.

And on a side note, some web2 CTO friends added linters to prevent their teams from using maps. Even in web2, indeterministic maps were too confusing with flappy unit tests or bugs discovered in production. They told me that maps are generally deficient in performance, not only in go. But please, keep this comment as a hint and not a fact.


My current feeling:

  • We should not deny them thoroughly (I hope we can one day find a technical and legal way to convert and import web2 libraries that make sense).
  • We should make them determinist.
  • We should/could deny them from being used as global values, function parameters, or returned values. So they can only be used for some patterns and inside functions, i.e., removing duplicates from a slice.
  • we should create better structures like p/avl.Tree and p/avl.MutTree to have easy-to-use data structs optimized for chains.
  • If we deny maps completely, we can suggest alternative approaches like []struct{Key:..., Value:...}, which is the most common approach for other languages not having map support or looking for more excellent compatibility.

@ajnavarro ajnavarro added 🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related and removed bug labels May 15, 2023
tbruyelle added a commit to tbruyelle/gno that referenced this issue Jun 26, 2023
Fix gnolang#817

There was actually 2 bugs in the code responsible for loading a map from
the state:
- `UnmarshalAmino` doesn't check that `ml.Tail` is nil before accessing
  it. It is correctly done for instance in `MapList.Append` for
  instance. The fix just ensures that `ml.Tail` is not nil.
- `fillTypesOfValue` which is called just after the store data is
  unmarshalSized, resets the map data by calling `MapValue.MakeMap()`.
  So at the end the map is always empty, whatever was stored before the
  transaction. The fix replaces the call of `MakeMap` to a simple map
  creation.

This has to be confirmed, but this change may also fix gnolang#311, because I'm
not convinced that the indetermenistic behavior of the golang map can be
a problem when saved to the store. Indeed, a gno map is serialized with
the help of a slice, which should remove any indetermenistic problem.
tbruyelle added a commit to tbruyelle/gno that referenced this issue Jun 26, 2023
Fix gnolang#817

There was actually 2 bugs in the code responsible for loading a map from
the state:
- `UnmarshalAmino` doesn't check that `ml.Tail` is nil before accessing
  it. It is correctly done for instance in `MapList.Append` for
  instance. The fix just ensures that `ml.Tail` is not nil.
- `fillTypesOfValue` which is called just after the store data is
  unmarshalSized, resets the map data by calling `MapValue.MakeMap()`.
  So at the end the map is always empty, whatever was stored before the
  transaction. The fix replaces the call of `MakeMap` to a simple map
  creation.

This has to be confirmed, but this change may also fix gnolang#311, because I'm
not convinced that the indetermenistic behavior of the golang map can be
a problem when saved to the store. Indeed, a gno map is serialized with
the help of a slice, which should remove any indetermenistic problem.
@moul moul closed this as completed in #932 Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants