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

vm,dao: return storage iterator from DAO in Storage.Find interop #992

Merged
merged 3 commits into from
May 27, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented May 27, 2020

Fix #988 .

Reproduce behavior of the reference realization:

  • if item was Put in cache after it was encountered during
    Storage.Find, it must appear twice
  • checking if item is in cache must be performed in real-time
    during Iterator.Next()

Need too retest #822 .

@fyrchik
Copy link
Contributor Author

fyrchik commented May 27, 2020

Linter is unhappy, because we return unexported type. But it is intended to be opaque. Maybe we should just return an interop item.

@roman-khimov
Copy link
Member

Or maybe we can move storageWrapper out of vm? I don't think it should be a part of vm.

@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #992 into master-2.x will decrease coverage by 0.12%.
The diff coverage is 38.33%.

Impacted file tree graph

@@              Coverage Diff               @@
##           master-2.x     #992      +/-   ##
==============================================
- Coverage       67.97%   67.85%   -0.13%     
==============================================
  Files             145      146       +1     
  Lines           14320    14359      +39     
==============================================
+ Hits             9734     9743       +9     
- Misses           4126     4155      +29     
- Partials          460      461       +1     
Impacted Files Coverage Δ
pkg/core/blockchain.go 44.81% <0.00%> (ø)
pkg/core/dao/cacheddao.go 37.00% <0.00%> (-5.21%) ⬇️
pkg/core/dao/dao.go 49.84% <0.00%> (ø)
pkg/core/interop_system.go 36.57% <0.00%> (ø)
pkg/vm/interop_iterators.go 95.23% <ø> (ø)
pkg/core/interop_neo.go 57.27% <75.00%> (-0.48%) ⬇️
pkg/core/storage_find.go 88.23% <88.23%> (ø)
pkg/vm/interop.go 90.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79c87ca...b0d07c3. Read the comment docs.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pkg/vm/kek?

@roman-khimov
Copy link
Member

roman-khimov commented May 27, 2020

Confirming that it actually fixes #977 too:

2020-05-27T11:30:15.241+0300    INFO    runtime log     {"script": "acdbf5346b59185b0e0df88fead2a7e856df1ab5", "logs": "\"Transfer is failed. fromBalance < value\""}
2020-05-27T11:30:15.241+0300    WARN    contract invocation failed      {"tx": "61366ab9fe37e100854446ab30005d7afbb093096670592ed8d499e3b6a6f753", "block": 2400907, "error": "error encountered at instruction 79 (THROWIFNOT): THROWIFNOT"}

Quick application log dump:

&{53f7a6b6e399d4d82e5970660993b0fb7a5d0030ab46448500e137feb96a3661 Application FAULT 5.645 [] []}

fyrchik added 3 commits May 27, 2020 11:40
Reproduce behavior of the reference realization:
- if item was Put in cache after it was encountered during
  Storage.Find, it must appear twice
- checking if item is in cache must be performed in real-time
  during `Iterator.Next()`
All storage items can still be retrived via zero-length prefix.
There is nothing wrong with iterators being implemented in other parts
of code (e.g. Storage.Find). In this case type assertions can
prevent bugs at compile-time.
@roman-khimov roman-khimov merged commit 0da1935 into master-2.x May 27, 2020
@roman-khimov roman-khimov deleted the fix/state branch May 27, 2020 08:53
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
None yet
Development

Successfully merging this pull request may close these issues.

2 participants