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: make slots public #3677

Merged
merged 1 commit into from
Nov 15, 2024
Merged

vm: make slots public #3677

merged 1 commit into from
Nov 15, 2024

Conversation

ixje
Copy link
Contributor

@ixje ixje commented Nov 13, 2024

@ixje
Copy link
Contributor Author

ixje commented Nov 14, 2024

this will fail signed off because it doesn't like my format, looking at how to fix the commit :|

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.

It would be nice to add a link to the reason of this modification to the commit message (#3674 (comment)).

LGTM, but not merging without @roman-khimov's approval.

@AnnaShaleva
Copy link
Member

looking at how to fix the commit :|

Expected "Erik van den Brink <git@erikvandenbrink.com>", but got "ixje <erik@coz.io>"

If you don't like the suggestion, then we can merge with this job failing, it's not critical.

@AnnaShaleva AnnaShaleva added this to the v0.107.0 milestone Nov 14, 2024
@AnnaShaleva AnnaShaleva added the vm VM tasks/bugs/issues label Nov 14, 2024
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.02%. Comparing base (dda2caf) to head (7ec0c11).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
cli/vm/cli.go 90.00% 1 Missing ⚠️
pkg/vm/context.go 85.71% 1 Missing ⚠️
pkg/vm/vm.go 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3677      +/-   ##
==========================================
- Coverage   83.09%   83.02%   -0.08%     
==========================================
  Files         334      334              
  Lines       46573    46612      +39     
==========================================
- Hits        38699    38698       -1     
- Misses       6306     6338      +32     
- Partials     1568     1576       +8     

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

@roman-khimov
Copy link
Member

looking at how to fix the commit

Whoever is in the "Author" of commit (git log, git config --get user.email) should be in SOB (at least the email should be the same).

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.

I'd suggest deprecating DumpXSlot at the same time, it was always questionable.

@ixje
Copy link
Contributor Author

ixje commented Nov 14, 2024

I'd suggest deprecating DumpXSlot at the same time, it was always questionable.

Those are in use by the cli

neo-go/cli/vm/cli.go

Lines 650 to 655 in cb51eeb

case "sslot":
rawSlot = vmCtx.DumpStaticSlot()
case "lslot":
rawSlot = vmCtx.DumpLocalSlot()
case "aslot":
rawSlot = vmCtx.DumpArgumentsSlot()

You want to remove these CLI commands?

@roman-khimov
Copy link
Member

You want to remove these CLI commands?

Of course not, they can grab slots from VM as is and represent them to user in any way they want including JSON.

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.

Last comments, otherwise LGTM.

…Slot methods

Signed-off-by: ixje <erik@coz.io>
@roman-khimov roman-khimov merged commit 895a0ae into nspcc-dev:master Nov 15, 2024
33 of 34 checks passed
@ixje ixje deleted the expose-slots branch November 15, 2024 10:28
@ixje ixje mentioned this pull request Dec 3, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm VM tasks/bugs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants