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

Custom operations plugin #1926

Merged
merged 20 commits into from Sep 18, 2019
Merged

Conversation

oxarbitrage
Copy link
Member

@oxarbitrage oxarbitrage commented Aug 16, 2019

New object type:

  • account_storage_object
    struct account_storage_object : public abstract_object<account_storage_object>
    {
       static const uint8_t space_id = CUSTOM_OPERATIONS_SPACE_ID; // = 7
       static const uint8_t type_id  = account_map; // = 0
    
       account_id_type account;
       string catalog;
       string key;
       optional<variant> value;
    };
    

New (embedded) operation type:

  • account_storage_map
    struct account_storage_map : chain::base_operation
    {
       bool remove;
       string catalog;
       flat_map<string, optional<string>> key_values;
    };
    
    Note:
    • serialize the account_storage_map operation (in binary form) and store it as the data field of custom_operation; the required_auths and id fields of custom_operation are ignored.
    • the "value" (of type optional<string>) in key_values is in JSON format.

New node API set custom_operations:

         /// @brief Retrieve the custom operations API
         fc::api<custom_operations_api> custom()const;
  • get_storage_info
           /**
            * @brief Get all stored objects of an account in a particular catalog
            *
            * @param account Account name to get info from
            * @param catalog Category classification. Each account can store multiple catalogs.
            *
            * @return The vector of objects of the account or empty
            */
           vector<account_storage_object> get_storage_info(std::string account, std::string catalog)const;
    
    
    Example:
    $ curl -d '{"id":1,"method":"call","params":["custom_operations","get_storage_info",["abit-test","cat1"]]}' https://testnet.xbts.io/ws
    {"id":1,"result":[{"id":"7.0.0","account":"1.2.3833","catalog":"cat1","key":"mykey","value":{"a":"b"}}]}
    

Wallet command / API:

  • account_store_map
        /**
         * Manage account storage map(key->value) by using the custom operations plugin.
         *
         * Each account can optionally add random information in the form of a key-value map
         * to be retrieved by any interested party.
         *
         * @param account The account ID or name that we are adding additional information to.
         * @param catalog The name of the catalog the operation will insert data to.
         * @param remove true if you want to remove stuff from a catalog.
         * @param key_values The map to be inserted/removed to/from the catalog
         * @param broadcast true if you wish to broadcast the transaction
         *
         * @return The signed transaction
         */
        signed_transaction account_store_map(string account, string catalog, bool remove,
              flat_map<string, optional<string>> key_values, bool broadcast);
    
    Example:
    >>> account_store_map abit-test cat1 false [["mykey","{\"a\":\"b\"}"]] true
    
  • get_account_storage
        /**
         * Get \c account_storage_object of an account by using the custom operations plugin.
         *
         * Storage data added to the map with @ref account_store_map and list data added by
         * @ref account_list_accounts will be returned.
         *
         * @param account Account ID or name to get contact data from.
         * @param catalog The catalog to retrieve.
         *
         * @return An \c account_storage_object or empty.
         */
        vector<account_storage_object> get_account_storage(string account, string catalog);
    
    Example:
    >>> get_account_storage abit-test cat1                                      
    [{
        "id": "7.0.0",
        "account": "1.2.3833",
        "catalog": "cat1",
        "key": "mykey",
        "value": {
          "a": "b"
        }
      }
    ]
    

-- below is the original (stale) message --
For issue #1847

Specifically for the issue operation `account_contact_operation` was created.

For HTLC orderbook(unrelated to the initial issue) operations create_htlc_order_operation and take_htlc_order_operation were created.

@abitmore helped with directions, we decided it is good enough for a draft pull request.

Open to feedback and questions, more details and how to use will be provided in an information BSIP i am starting to write.

@abitmore abitmore added this to the 4.1.0 - Feature Release milestone Aug 16, 2019
@abitmore abitmore added this to In development in Protocol Upgrade Release (5.0.0) via automation Aug 16, 2019
@syalon
Copy link
Member

syalon commented Aug 21, 2019

I think, we can provide an object, just store the key-value data structure for the account, it can be easily used for other purposes. We can provide standards for many general purposes, but I think we can also provide some custom data reading and storage.

So I wonder if we can provide an operation like the following:

account_store_data(user_key, user_value)
account_fetch_data(user_key)
account_fetch_data_all
account_delete_data(user_key)
account_delete_data_all

In this way, we can satisfy specific purposes and some more general purposes.

@oxarbitrage
Copy link
Member Author

I will add this functionality as an account_store object. Thank you @syalon

@syalon
Copy link
Member

syalon commented Aug 21, 2019

@oxarbitrage It should be me thank you, I just put forward some requirements, and you need to implement it.

Another question.
Current the contact object seems to be storing information about itself.
Not the contact list data associated with an account?
For example: my account is abc, I want to add alice, bob to my contact list. In this way, when I transfer money, there will be no possibility of misoperation.

@abitmore
Copy link
Member

For easier expanding, we can define an operation to update data stored in a generic account-data map, which takes a string as key, and another string as value. We can have multiple key-value pairs in the operation. The operation replaces values in the map.

For use cases like white-listing, a string is not enough for the value, ideally we need a set, because it's inconvenient to replace a whole list, the operation should add/remove items to/from the set with a given key. I think it's better to define this as a new operation.

I think having these 2 operations will fit most of data-storage needs.

By the way, we can probably (ab)use the id field of custom_operation for some use cases.

@oxarbitrage
Copy link
Member Author

7e59910

New account_storage_object was added with members:

   account_id_type account;
   flat_map<string, string> storage_map;
   flat_set<account_id_type> account_list;

Operations to manage them were added, this are:

  • account_store_data to manage the key-value storage map.
  • account_list_data to manage the account list set.

Test cases were added at

BOOST_AUTO_TEST_CASE(custom_operations_account_storage_test)

There is a limitation of only update the map or the account list with 10 rows of data at a time. I am thinking on limiting the overall map and set in the object to 100 values or similar.

I am wondering now if we should just host storage_map and account_list in the contact object instead of having its own. Open to feedback/change here.

Wallet calls are not added yet.

@oxarbitrage
Copy link
Member Author

After a discussion with @abitmore about the last commit there are some issues with the underlying object implementation that we decided to change.
We are changing the storage object to just:

   account_id_type account;
   string key;
   string value;

This will allow us to create indexes and search by key, instead of one big object per account with map and set, we will have multiple key-value objects per account. Operations will remain unchanged if possible, one to add data in the form of key-value and another to add just a key for a list.

Will submit those changes as soon as possible, working on them now.

@abitmore
Copy link
Member

another to add just a key for a list

This is confusing.

My idea is something like this:

account key value
acc1 key1 unique-item-1
acc1 key2 list-item-1
acc1 key2 list-item-2
acc2 key1 unique-item-2

So, query for (acc1,key1) will get a unique item, query for (acc1,key2) will get a list.

I'm not sure whether it's a good idea to store key-value data and key-list data together.

For key-value data, we need

  • an operation to set or update value of a key, and
  • an operation to set or update multiple key-value pairs, and
  • an operation to remove one key or multiple keys, and
  • perhaps an operation to remove all keys.

Some of the operations listed above can be combined.

Using string type for keys is reasonable due to flexibility. Since we need to index it, for performance, I think we need to limit length of keys.
For values, generally, using string would need more spaces, but should be acceptable due to better readability. Here is an example about how to store data in different types directly:

using argument_type = fc::static_variant<GRAPHENE_OP_RESTRICTION_ARGUMENTS_VARIADIC>;

For key-list data, we need

  • an operation to add multiple values into a list for a key, and
  • an operation to remove multiple values from a list for a key, and
  • an operation to remove one key or multiple keys, and
  • perhaps an operation to remove all keys.

For dedupe, we need to index values as well; for performance, we need to limit values' length, but this is not ideal. Perhaps a better approach for key-list data is to store it as (account, key, sub-key, value), set limitations on key and sub-key, and index them, but don't index values; also, for some use cases, value can be empty.

@syalon
Copy link
Member

syalon commented Aug 24, 2019

struct account_storage_object : public abstract_object<account_storage_object>
{
    static const uint8_t space_id = CUSTOM_OPERATIONS_SPACE_ID;
    static const uint8_t type_id = account_store;

    account_id_type account;
    string catalog;
    optional<string > key;
    string value;
};

I think this structure can satisfy most situations.
We can index the account, or we can combine the index account and catalog.

  • account - the account id of the data you want to store
  • catalog - category classification. for example: contact list, favorite trading-pairs, etc.
  • key - this is an optional field. If this field exists, we can treat it as a map structure. If the field does not exist, we treat it as a vector structure.
  • value - the actual data.

In this way, we implement most of the data structures: map, vector (the element can be repeated), set (the element can not be repeated, all map keys).

@oxarbitrage
Copy link
Member Author

Changed the object to structure suggested by @syalon at 1260169

2 operations to manage(add/edit/delete) a map or a list:

  • account_storage_map
  • account_storage_list

@abitmore
Copy link
Member

abitmore commented Aug 24, 2019

@syalon the "does not exist" key is not the way to store a vector. For storing data, the tuple (account,catalog,key) must be unique. If you want to store duplicate values in a catalog, and you want to keep the data in your order, you need to have an additional unique key as helper for sorting as well as for data modification and removal. Alternatively, you can store the whole vector as a single JSON string or whatever you can parse directly in the value field, then the operation for modification will need to be bigger, because need to update with a new string containing the whole list every time.

An optional key in DB is hard for designing API, because an optional parameter in API usually means something else.

If change the key type away from optional, then the data structure is the same to my conclusion in the last comment (I mentioned (account, key, sub-key, value)).

By the way, personally I think "category" is a better word than "catalog" here. I could be wrong though.

@oxarbitrage
Copy link
Member Author

What is the use case of storing a vector that we cant do with a set or map as we have now ?

@syalon
Copy link
Member

syalon commented Aug 26, 2019

Maybe, we only consider set and map. I haven't thought of any scenes that need a vector. @oxarbitrage

@pmconrad
Copy link
Contributor

How about storing values as variant? This way it could be a list, an object, a string, a number, ...
The plugin doesn't have to care what the value is.

@abitmore
Copy link
Member

Using variant saves storage space for certain data types, but adds computation overheads for API nodes (for deserialization / stringnification).

Strings are easier for client apps to integrate so far, IIRC bitsharesjs doesn't support variant.

@oxarbitrage
Copy link
Member Author

I have the code to change the value of the account_storage_object into variant ready to push however lets first agree on if we want it.

@syalon
Copy link
Member

syalon commented Aug 31, 2019

I also think that string is ok. We should keep it simple.

@jmjatlanta
Copy link
Contributor

I want to verify that conceptually I have the desired outcome correct. Please fix what I have wrong.

The idea here is to use custom operations to store data on the chain. We have some examples of the type of data in this PR (i.e. account holder data, htlc data).

The API nodes with the plugin enabled will be able to serve the information. API nodes without the plugin enabled will reject the request.

If the above two statements are correct, I worry about the pollution of the API. With this PR, we are asking those hosting nodes to support an ever expanding list of specialized API calls, and developers to maintain it.

If the API could be more "generic", whereby changes to the data structure are unrelated to a small list of API methods, I would imagine required maintenance would be much less. Otherwise I see each new use case needing additions to api.hpp. I realize this shifts development efforts elsewhere, but I believe it also provides those that want truly custom data on the chain to get it there and retrieve it without changes in core.

But before I go about redesigning everything that everyone else has worked on, perhaps I should wait on the BSIP, and get a clear picture of the desired outcome. Please feel free to school me on why this was done this way.

@oxarbitrage
Copy link
Member Author

We had done that. There are 2 specific use cases of additional account data information and htlc offers that were the first scenarios we had in mind.
Then generic storage was added with 2 new operations to save key=>value data or list of strings. Each account can have several catalogs for different purposes by the addition of these.

This should cover the use cases we can think of right now and with the general storage there should be no need to add more api calls, the ones we currently have will probably need to be refined but they will not grow with each case.

We always have the option to add more objects and operations if an specific use case came in just as we did for htlc and contact info but ideally we will like to cover the generic use cases with the account storage operations and objects where possible.

The account data case can be achieved by using the storage map so we could remove if we want, however by now we are keeping it as a way to have a standard mean to add this data specifically. We will not want that some wallet adds a contact catalog for it users while other wallet having a contact_information to do the same thing.

The htlc use case can not be done by the generic storage options as it is now.

Hope it clarifies.

@pmconrad
Copy link
Contributor

pmconrad commented Sep 4, 2019

We should keep it simple.

Yes. I think almost everything would be much simpler with the variant approach.

The API could be completely generic. Clients would see the content as JSON, which is more powerful than strings and therefore simpler to use for any use-case requiring structured data.

Both htlc offers and account info are specific applications of the generic functionality. Their dedicated API calls can simply be removed.

Well-known keys and value structures can be specified in Protocol (client) BSIPs, with no further implementation effort.

The only thing that gets more complicated is the serialization of variant when signing the custom_operation, but that can be worked around by providing a helper API call that takes a JSON structure and returns a serialized op.

@oxarbitrage
Copy link
Member Author

The value of the storage object is now a variant. We cant index that which it is fine for the map but inconvenient for the list. So subkey was added as a string and lists can be stored there. Lists will not accept a variant but just a string as before.
Size of catalog, key and subkey is limited to 200 chars as they are indexed and we don't want them to be too long. Variant value haves no limitation.

We have now only 2 operations to manage the object by manipulating account map or lists.

Commit is here e4f0d76 but as many stuff was removed i think it will be easier to check the whole thing again.

For HTLC offers we cant do the previous functionality(create-take offers) with general storage, basically because we cant modify an object created by another account.
I was talking with @pmconrad about this problem and he haves the idea that we can store the offer with general storage, we can specify a special HTLC catalog for it and the matching can be done automatically by the plugin when normal htlc_create operations are done. This will save 1 step of taking the offer manually.

So, for that to happen we need to plugin to be listening to htlc_create operations that are happening in the chain. This will be optional as probably some plugin users will not be interested on that but only in the store functionality. I still didn't think too much about the potential problems this implementation can have as i am more interested on having the general storage part done first but open to comments.

tests/CMakeLists.txt Outdated Show resolved Hide resolved
Protocol Upgrade Release (5.0.0) automation moved this from In development to In testing Sep 18, 2019
@qualwebs
Copy link

Changed the object to structure suggested by @syalon at 1260169

2 operations to manage(add/edit/delete) a map or a list:

* `account_storage_map`

* `account_storage_list`

Hello bitshares community, I have some queries regarding the account_storage_list functionality as its not properly working on the CLI wallet.

As I had found it somewhere that these operations were renamed to - account_store_map and get_account_storage which is indeed correct. However, the get_account_storage isn't working at all.

How its supposed to work : Return the account_storage_object of specified account stored using account_store_map
How its working : I ALWAYS get an empty array as the ouput - [].

See the below example with images :-

Step-1 : Storing data using - account_store_map

Screenshot from 2024-01-24 12-39-15

  • I see the transaction is made which probably means the data is stored in the account of nathan

Step-2 : However, when I try to retrieve data using get_account_storage , I get only an empty array as output.

Screenshot from 2024-01-24 12-43-07

PS: I would also like to contribute towards this community. I find it really amazing how bitshares facilitates very less average transaction time compared to any other blockchain. I am eager to play a part in contributing towards this community. Please do consider the issue with get_account_storage or if I got something wrong.

@abitmore
Copy link
Member

FWIW I think the above question was solved here: https://github.com/orgs/bitshares/discussions/2804#discussioncomment-8242384.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants