Skip to content

GetInvocationCounter#813

Merged
shargon merged 12 commits intoneo-project:masterfrom
shargon:invocation-counter
Jun 13, 2019
Merged

GetInvocationCounter#813
shargon merged 12 commits intoneo-project:masterfrom
shargon:invocation-counter

Conversation

@shargon
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@vncoelho vncoelho Jun 11, 2019

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
Copy Markdown
Member Author

@shargon shargon Jun 11, 2019

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
Copy Markdown
Member

@vncoelho vncoelho Jun 11, 2019

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
Copy Markdown
Member

@vncoelho vncoelho Jun 11, 2019

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
Copy Markdown
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

igormcoelho
igormcoelho reviewed Jun 11, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

igormcoelho commented Jun 11, 2019

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

@shargon
Copy link
Copy Markdown
Member Author

shargon commented Jun 11, 2019

Is not related, is according to this issue #307

erikzhang
erikzhang reviewed Jun 12, 2019
View reviewed changes
@shargon
Copy link
Copy Markdown
Member Author

shargon commented Jun 12, 2019

@erikzhang could you compute the gas cost?

@codecov-io
Copy link
Copy Markdown

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
@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 <erik@neo.org>

* Update neo/SmartContract/InteropService.cs

Co-Authored-By: Erik Zhang <erik@neo.org>

* 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

@erikzhang erikzhang erikzhang approved these changes

+1 more reviewer

@igormcoelho igormcoelho igormcoelho left review comments

Reviewers whose approvals may not affect merge requirements

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