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

Remove empty keys #825

Merged
merged 8 commits into from Jun 15, 2019

Conversation

5 participants
@shargon
Copy link
Member

commented Jun 13, 2019

Close #824

@shargon shargon requested review from igormcoelho and erikzhang Jun 13, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 13, 2019

Codecov Report

Merging #825 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #825      +/-   ##
=========================================
- Coverage   38.41%   38.4%   -0.02%     
=========================================
  Files         176     176              
  Lines       12459   12463       +4     
=========================================
  Hits         4786    4786              
- Misses       7673    7677       +4
Impacted Files Coverage Δ
neo/SmartContract/InteropService.cs 20.89% <0%> (-0.19%) ⬇️

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 fe3ab04...815fcb4. Read the comment docs.

shargon and others added some commits Jun 13, 2019

// If is empty, we try to remove it

engine.Snapshot.Storages.Delete(skey);
return true;

This comment has been minimized.

Copy link
@erikzhang

erikzhang Jun 13, 2019

Member

If I put an empty bytes with StorageFlags.Constant flag, what will happen?

This comment has been minimized.

Copy link
@shargon

shargon Jun 13, 2019

Author Member

The expected behaviour is: write nothing, read empty bytes

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Jun 13, 2019

Contributor

What's an example use case for const? I think you're right... logically speaking, if someone wants to keep a constant value, that value could be zero. So we need to keep it allow it to be unchanged in the future. We cannot pre-reap const empty values @shargon, because it may be part of contract logic, and perhaps be re-written in the future because we accidentally pre-dropped its "constness".

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Jun 13, 2019

Contributor

Only explicit Delete will destroy them, right @erikzhang ? I just noticed Delete cannot reap const values too. So these are contract-lifetime values, correct? Only Contract.Destroy or Migrate can reap them.

igormcoelho added some commits Jun 13, 2019

@igormcoelho
Copy link
Contributor

left a comment

Looks good for me, a huge advance for NEP-5!

@igormcoelho

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@shargon this will guarantee that value zero will always be converted to an empty array, thus, ripped out automatically 💪
neo-project/neo-vm#171

@vncoelho

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Such a nice PR and quick, Shargon.
Congratulations to you and Igor for the insight and proposal.

@erikzhang erikzhang added this to the NEO 3.0 milestone Jun 14, 2019

@igormcoelho

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@erikzhang I just noticed this will be useless for NEP5 Native Tokens, because each Key is being mapped to a TState object, not a BigInteger as I thought before...
Although this will still be fundamental for user-made NEP5, I wonder if we could directly put amount in a user key, instead of TState object? And perhaps, put TState somewhere else, or even remove it? Votes and Gas redistribution could still be made in connected contracts, this way is too attached to apply optimizations like the ones I was thinking. This will also be a challenge if we want to reuse Native NEP5 sketch for hand-made tokens, as in the inheritance proposal.

@igormcoelho

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

This PR could be very helpful: #831
Issue: #830

@shargon

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

I think that i have an optimization

item.Value = value;
item.IsConstant = flags.HasFlag(StorageFlags.Constant);

if (engine.Snapshot.Storages.TryGet(skey)?.IsConstant == true) return false;

This comment has been minimized.

Copy link
@shargon

shargon Jun 15, 2019

Author Member

If we could access here to TrackableItem, we can save the GetAndChange

This comment has been minimized.

Copy link
@erikzhang

erikzhang Jun 15, 2019

Member

GetAndChange here doesn't access the internal storage. It read from the cache because we call TryGet before.

This comment has been minimized.

Copy link
@shargon

shargon Jun 15, 2019

Author Member

But if we can know if the item has trackable.State = TrackState.Added; we can avoid the two reads

This comment has been minimized.

Copy link
@erikzhang

erikzhang Jun 15, 2019

Member

If it is TrackState.Added, both the TryGet and GetAndChange calls read from the cache. These are all automatic.

@shargon shargon merged commit d411153 into neo-project:master Jun 15, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shargon shargon deleted the shargon:delete-on-empty branch Jun 15, 2019

KickSeason added a commit to KickSeason/neo that referenced this pull request Jul 9, 2019

Remove empty keys (neo-project#825)
* Close neo-project#824

* optimize

* remove redundancy

* simplified

* allow const empty array

* Update InteropService.cs

* Update InteropService.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.