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.

Sign up for GitHub

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

Jump to bottom

Allow to partially unwrap session iterator #3274

Merged
merged 4 commits into from
Dec 29, 2023
Merged

Allow to partially unwrap session iterator #3274

merged 4 commits into from
Dec 29, 2023
+271 −21

Conversation

fyfyrchik
Copy link
Contributor

@fyfyrchik fyfyrchik commented Dec 19, 2023 •
edited

Close #3272

@fyfyrchik fyfyrchik changed the title Allow to partially unwrap session iterator WIP: Allow to partially unwrap session iterator Dec 19, 2023
@fyfyrchik fyfyrchik marked this pull request as draft December 19, 2023 17:37
@fyfyrchik fyfyrchik changed the title WIP: Allow to partially unwrap session iterator Allow to partially unwrap session iterator Dec 19, 2023
@fyfyrchik fyfyrchik force-pushed the partial-iterator branch 2 times, most recently from 7018c1c to ec0de6e Compare December 19, 2023 17:41
@fyfyrchik
Copy link
Contributor Author

fyfyrchik commented Dec 19, 2023 •
edited

A good question is where to put tests for this? I haven't found anything for another CreateCall* script.

@fyfyrchik fyfyrchik marked this pull request as ready for review December 20, 2023 10:17
@fyfyrchik fyfyrchik force-pushed the partial-iterator branch 7 times, most recently from aa71f43 to eba7385 Compare December 20, 2023 10:42
AnnaShaleva
AnnaShaleva previously approved these changes Dec 20, 2023
View reviewed changes
pkg/smartcontract/entry.go Show resolved Hide resolved
pkg/smartcontract/entry.go Show resolved Hide resolved
pkg/smartcontract/entry.go Show resolved Hide resolved
pkg/smartcontract/entry.go Show resolved Hide resolved
pkg/rpcclient/unwrap/unwrap.go Outdated Show resolved Hide resolved
pkg/rpcclient/unwrap/unwrap.go Outdated Show resolved Hide resolved
pkg/rpcclient/unwrap/unwrap.go Outdated Show resolved Hide resolved
pkg/vm/iterator_test.go Show resolved Hide resolved
@AnnaShaleva AnnaShaleva dismissed their stale review December 20, 2023 15:45

Approve is accedent, I was going to push "Comment" button.

@AnnaShaleva
Copy link
Member

And just a side note: what do you think about extending "getversion" RPC call to be able to query RPC server settings? Then we may include SessionEnabled and a set of other RPC-specific settings to the response.

@roman-khimov
Copy link
Member

Makes sense for things that can probed from outside anyway.

@AnnaShaleva AnnaShaleva mentioned this pull request Dec 20, 2023
Closed
@fyfyrchik fyfyrchik force-pushed the partial-iterator branch 2 times, most recently from 4c4cd7d to e6bfe73 Compare December 21, 2023 15:41
roman-khimov
roman-khimov reviewed Dec 25, 2023
View reviewed changes
pkg/rpcclient/unwrap/unwrap.go Outdated Show resolved Hide resolved
pkg/rpcclient/unwrap/unwrap_test.go Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Dec 26, 2023 •
edited

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (3176f72) 85.16% compared to head (78050e8) 85.25%.

Files Patch % Lines
pkg/smartcontract/entry.go 92.50% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3274      +/-   ##
==========================================
+ Coverage   85.16%   85.25%   +0.08%     
==========================================
  Files         327      327              
  Lines       44267    44326      +59     
==========================================
+ Hits        37702    37790      +88     
+ Misses       5071     5039      -32     
- Partials     1494     1497       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AnnaShaleva
Copy link
Member

Linter job is unhappy about increment.

@AnnaShaleva
Copy link
Member

@fyfyrchik, could you fix the issues that are left? We'll make it a part of 0.105 and release within a several days.

@AnnaShaleva AnnaShaleva mentioned this pull request Dec 28, 2023
Merged
@fyfyrchik
smartcontract: Add CreateCallAndPrefetchIteratorScript()
ba14173 
There is CreateCallAndUnwrapIteratorScript() which can traverse
iterator for nodes with sessions disabled. For other nodes this may
still be beneficial: if there is a small number of items, we might read
(or prefetch) all of them in one request. However, this script continues
to work even for large collections, returning both accumulated array and
remaining iterator.

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
@fyfyrchik
unwrap: Add ArrayAndSessionIterator(), close nspcc-dev#3272
402a73b 
It can be used to work with the results of
CreateCallAndPrefetchIteratorScript() execution. The first item must be
an array and the optional second item must be an iterator, containing
remaining elements.

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
@fyfyrchik
vm: Add tests for iterator unwrap scripts
4b7b71c 
Check that is uses only 3 syscalls and also specify boundary behaviour.

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
@fyfyrchik
unwrap: Add test for HALT state with an empty stack
78050e8 
Missing piece for perfect coverage. Check that an error is returned an
no panic occurs.

Signed-off-by: Evgenii Stratonikov <fyfyrchik@runbox.com>
AnnaShaleva
AnnaShaleva approved these changes Dec 29, 2023
View reviewed changes
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

💖

@AnnaShaleva AnnaShaleva merged commit 0d3f749 into nspcc-dev:master Dec 29, 2023
18 of 19 checks passed
@roman-khimov roman-khimov mentioned this pull request Jan 18, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov left review comments

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow to partially unwrap session iterator
3 participants
@fyfyrchik @AnnaShaleva @roman-khimov