Skip to content
This repository has been archived by the owner on Nov 15, 2021. It is now read-only.

Add initial Storage Iterator implementation #339

Closed
wants to merge 8 commits into from
Closed

Add initial Storage Iterator implementation #339

wants to merge 8 commits into from

Conversation

ixje
Copy link
Member

@ixje ixje commented Mar 18, 2018

What current issue(s) does this address, or what feature is it adding?
Fixes #211

This adds the new Storage Iterator API for smart contracts. Note that we should wait with merging until the C# side is fixed (see the issue linked in #211). I wanted to get the feedback loop started while the C# side sorts out their implementation.

How did you solve this problem?
Port code

How did you make sure your solution works?
Compiled a contract in C#, invoked using neo-python. Default tests currently fail because it requires updating the fixtures (see reason below) which I'm waiting with until the C# side is finalised + feedback here is finalised.

Are there any special changes in the code that we should be aware of?
Yes, the leveldb keys for the Storage Put/Get/Delete calls have been altered to make prefix searching possible (discussed with @localhuman) which requires rebuilding the chain + recreating the fixtures.

Please check the following, if applicable:

  • Did you add any tests?
  • Did you run make lint?
  • Did you run make test?
  • Are you making a PR to a feature branch or development rather than master?
  • Did you add an entry to CHANGELOG.rst? (if not, please do)

@localhuman
Copy link
Collaborator

This is a great start! Hopefully things get fixed on the C# side ...

@ixje
Copy link
Member Author

ixje commented Mar 22, 2018

So it seems like the C# side is fixed with this commit; neo-project/neo@6d5cf63

Most of the changes are just specific to their database wrapper so to speak. One thing I was 'worried' about for a moment are the changes to the serialize and deserialize methods of StorageKey. A quick reference search doesn't really show them to be used. I also did a caveman approach by putting a breakpoint on both routines and then sync up my testnet from 930K to ~980. No breakpoint was hit so I don't think it's used anywhere, but it might be good if the reader can also give it some thought if it could be used somewhere now (or possibly in the future).

If after this all you still think that the serialize/deserialize changes don't affect us, please let me know so I can go and update the fixtures and write tests

@localhuman
Copy link
Collaborator

I don't think we'll need or use Serialize or Deserialize for storage keys. We could make the StorageKey not inherit SerializableMixin and remove those methods so that nobody is tempted to use them in the future.

@ixje
Copy link
Member Author

ixje commented Apr 20, 2018

@localhuman has to upload the new fixtures. If the build fails because of fixtures_v6.tar.gz not found, please manually see if there file is already there and restart the build if you notice it before me :)

ixje and others added 3 commits April 25, 2018 17:11
 into sc_storage_iterator

* 'sc_storage_iterator' of https://github.com/ixje/neo-python: (42 commits)
  Fixed wrong privnet instructions
  Support pip version 10.0 and greater (#389)
  setup.py: fix shebang: python3
  removing all occurrences of: "# -*- coding:utf-8 -*-" (#386)
  lint fix for comments
  Modified NEP-5 token import test to also test token deletion
  Updated CHANGELOG.rst
  fix for NEP-5 token deletion from wallet
  update changelog
  update logging
  Update CHANGELOG.rst
  create SmartContractEvent with working block height instead of last
  persist NotifyType.REFUND events in notification DB
  remove print
  updated networking
  Bump version: 0.6.7 → 0.6.8-dev
  Bump version: 0.6.7-dev → 0.6.7
  Update changelog for release
  Added CHANGELOG entry
  Added CORS header Access-Control-Allow-Headers
  ...
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 79.174% when pulling 7563a65 on ixje:sc_storage_iterator into 0678cc2 on CityOfZion:development.

@ixje
Copy link
Member Author

ixje commented Apr 25, 2018

Hooray build passed! :)

So there's 3 things left

  1. somehow inform users that they'll have to resync their chain once they're on a version that includes these changes. How did we do that previous times?
  2. add a bootstrap file. Do you already hate me @localhuman 😆
  3. add neo-boa support so automated tests can be added. I currently have manually tested it by compiling and invoking the C# contract mentioned here: Add StorageIterator neo-project/neo-devpack-dotnet#19 (comment)

Now, for the boa support I can add a Find(context, needle) call to the Storage class, but that's not enough. Take something like this:

from boa.interop.Neo.Runtime import Notify
from boa.interop.Neo.Storage import GetContext, Put, Find

def Main(args):
    ctx = GetContext()
    Put(ctx, 'my_prefixA', "1")
    Put(ctx, 'my_prefixB', "2")
    Put(ctx, 'something', "3")

    result_iterator = Find(ctx, 'my_prefix')
    while result_iterator.Next():
        Notify(result_iterator.Key()) 

The compiler will complain it can't find Next() and Key(). I gave it a quick try but I just kept getting new errors so I think it's best if someone that understands the neo-boa architecture well would do it. cough that would be you @localhuman 😆 cough

@localhuman
Copy link
Collaborator

Sounds like a plan!

@localhuman
Copy link
Collaborator

Note, i've created a new branch here https://github.com/CityOfZion/neo-python/tree/feature-storage-iterator which have these changes. I needed to change a few things to get it working.

Also, do you have the compiled avm from C# which I can test with 😀

@localhuman
Copy link
Collaborator

Also, for reference see CityOfZion/neo-boa#70

@localhuman localhuman mentioned this pull request Apr 30, 2018
5 tasks
@localhuman
Copy link
Collaborator

closing in favor of #406

@localhuman localhuman closed this Apr 30, 2018
@ixje ixje deleted the sc_storage_iterator branch June 29, 2018 16:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants