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

GetInvocationCounter #813

Merged
merged 12 commits into from
Jun 13, 2019
Merged

GetInvocationCounter #813

merged 12 commits into from
Jun 13, 2019

Conversation

shargon
Copy link
Member

@shargon shargon commented Jun 11, 2019

Close #307

@shargon shargon requested a review from erikzhang June 11, 2019 18:02
vncoelho
vncoelho reviewed Jun 11, 2019
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@shargon, this is an interesting thing, is there any use that you have already discussed?

neo/SmartContract/InteropService.cs
}
else
{
engine.InvocationCounter[contract.ScriptHash] = 1;
Copy link
Member

Choose a reason for hiding this comment

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

Shargon, could this Dictionary be already initialized in 1 and avoid this if?

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to increment the value, so you need to query the previous value

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I mean, for the first time initialize already with 1 and then just increment.

Copy link
Member

Choose a reason for hiding this comment

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

This would simplify the if.

Copy link
Member Author

@shargon shargon Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's initialized to 1 when it is not found

neo.UnitTests/UT_Syscalls.cs Show resolved Hide resolved
igormcoelho
igormcoelho reviewed Jun 11, 2019
View reviewed changes
Copy link
Contributor

@igormcoelho igormcoelho left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Shargon I'll review, but we also have a related interesting and complementary solution to this.

@igormcoelho
Copy link
Contributor

@shargon can you take a look at #814 ? I think its related.

@shargon
Copy link
Member Author

shargon commented Jun 11, 2019

Is not related, is according to this issue #307

@shargon
Copy link
Member Author

shargon commented Jun 12, 2019

@erikzhang could you compute the gas cost?

@codecov-io
Copy link

codecov-io commented Jun 12, 2019 •
edited
Loading

Codecov Report

Merging #813 into master will increase coverage by 0.4%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #813     +/-   ##
=========================================
+ Coverage      38%   38.41%   +0.4%     
=========================================
  Files         176      176             
  Lines       12443    12459     +16     
=========================================
+ Hits         4729     4786     +57     
+ Misses       7714     7673     -41
Impacted Files Coverage Δ
neo/SmartContract/ApplicationEngine.cs 56.81% <100%> (+0.49%) ⬆️
neo/SmartContract/InteropService.cs 21.08% <86.66%> (+5.59%) ⬆️
neo/IO/Caching/DataCache.cs 61.27% <0%> (+4.04%) ⬆️
neo/VM/Helper.cs 18.7% <0%> (+9.67%) ⬆️
neo/Ledger/ContractState.cs 19.44% <0%> (+19.44%) ⬆️

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 354563d...52cfa25. Read the comment docs.

erikzhang
erikzhang reviewed Jun 12, 2019
View reviewed changes
neo/SmartContract/ApplicationEngine.cs Outdated Show resolved Hide resolved
neo/SmartContract/InteropService.cs Outdated Show resolved Hide resolved
@shargon shargon merged commit fe3ab04 into neo-project:master Jun 13, 2019
@shargon shargon deleted the invocation-counter branch June 13, 2019 08:41
@erikzhang erikzhang added this to the NEO 3.0 milestone Jun 13, 2019
Tommo-L pushed a commit to Tommo-L/neo that referenced this pull request Jun 22, 2020
@shargon @Tommo-L
GetInvocationCounter (neo-project#813)
776088d 
* GetInvocationCounter

* UT as sample

* Fix travis

* Update neo/SmartContract/InteropService.cs

* Update neo/SmartContract/ApplicationEngine.cs

Co-Authored-By: Erik Zhang <[email protected]>

* Update neo/SmartContract/InteropService.cs

Co-Authored-By: Erik Zhang <[email protected]>

* We need Writable dictionary

* internal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vncoelho vncoelho vncoelho left review comments

@igormcoelho igormcoelho igormcoelho left review comments

@erikzhang erikzhang erikzhang approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
NEO 3.0
Development

Successfully merging this pull request may close these issues.

New API for NeoContract: System.Runtime.GetInvocationCounter
5 participants
@shargon @igormcoelho @codecov-io @erikzhang @vncoelho