vm: fix packmap operation #3685
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3685 +/- ##
==========================================
- Coverage 83.05% 83.03% -0.03%
==========================================
Files 334 334
Lines 46599 46598 -1
==========================================
- Hits 38704 38693 -11
- Misses 6322 6327 +5
- Partials 1573 1578 +5 ☔ View full report in Codecov by Sentry. |
AliceInHunterland
force-pushed
the
vm-map
branch
from
709e948
to
fedfcaa
Compare
AliceInHunterland
requested review from
AnnaShaleva and
roman-khimov
as code owners
| }) | ||
| for i := 1; i < MaxSize/85; i++ { |
There was a problem hiding this comment.
Maybe MaxSerialized better
Close #3613 Signed-off-by: Ekaterina Pavlova <ekt@morphbits.io>
AliceInHunterland
force-pushed
the
vm-map
branch
from
fedfcaa
to
6b34149
Compare
| require.NoError(t, err) | ||
| require.NotNil(t, m) | ||
| require.Equal(t, 0, len(m)) | ||
|
|
| @@ -1214,15 +1214,14 @@ func (v *VM) execute(ctx *Context, op opcode.Opcode, parameter []byte) (err erro | |||
| panic("invalid length") | |||
| } | |||
|
|
|||
| items := make([]stackitem.MapElement, n) | |||
| for i := range n { | |||
| newMap := stackitem.NewMap() | |||
| key := v.estack.Pop() | ||
| validateMapKey(key) | ||
| val := v.estack.Pop().value | ||
| items[i].Key = key.value | ||
| items[i].Value = val | ||
| newMap.Add(key.Item(), val) |
There was a problem hiding this comment.
This way is not optimal, verification is performed twice.
| res := NewMap() | ||
| for i := range value { | ||
| if err := IsValidMapKey(value[i].Key); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
It's too risky to panic inside NewMapWithValue, it may be used outside the VM. Either adjust a documentation of this function and don't check for value correctness or change the signature of this function to return error. BTW, with the updated PACKMAP, do we really use this function anywhere?
There was a problem hiding this comment.
No, NewMapWithValue is used only in tests.
|
Add a designated compatibility test, please. |
Close #3613