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

Add A Fee Caching/Distribution System #359

Merged
merged 22 commits into from May 27, 2016

Conversation

Projects
None yet
2 participants
@zathras-crypto
Copy link

zathras-crypto commented Apr 13, 2016

This PR serves to add the concept of fees to the Omni Layer.

The main additions are:

  • A fee caching system to enable an accumulation of fees within the protocol but not held by any address until such time as a distribution is triggered
  • A fee distribution system to distribute the contents of a fee cache proportionally amongst all holders of the Omni token
  • A fee history system to enable data retrieval about completed fee distributions

The main consensus affecting changes are:

  • Code to slice 0.05% of trades of non-Omni pairs and move the tokens to the fee cache
  • Code to trigger fee distribution once the fee cache reaches 0.001% of the total tokens for that property (arbitrary value, we can change with OMNICORE_FEE_THRESHOLD in rules.h)

The testing is done via a regtest script (test/test_fees.sh) however this requires disabling Dev Omni and recompiling to run the tests. I was hoping the testing guys (@msgilligan @dexX7) might be able to advise on converting to the boost unit test framework instead.

Please note: this is not yet ready for merging, there are still a few TODOs left including setting this up for activation, it's primarily to start the review process

The RPC calls made available with this PR are as follows:

omni_getfeeshare
Use this call to query what percentage of fees a particular address will receive at that time. Supply an address parameter to query any address, or do not supply parameters to query the wallet.

omni_getfeeshare [ address ]

Returns the percentage share of fees distribution applied to the wallet (default) or address (if supplied).

Arguments:
1. address              (string, optional) retrieve the fee share for the supplied address

Result:
[                       (array of JSON objects)
  {
    "propertyid" : nnnnnnn,        (number) the property id
    "cachedfees" : "n.nnnnnnnn",   (string) the amount of fees cached for this property
  },
  ...
]

omni_getfeecache
Use this call to determine the current value of the fee cache. Without parameters it will display all properties with non-zero fee caches. Supply a property ID to display the fee cache for a specific property.

omni_getfeecache [ propertyid ]

Returns the amount of fees cached for distribution.

Arguments:
1. propertyid           (number, optional) filter the results on this property id

Result:
[                       (array of JSON objects)
  {
    "propertyid" : nnnnnnn,        (number) the property id
    "cachedfees" : "n.nnnnnnnn",   (string) the amount of fees cached for this property
  },
...
]

omni_getfeetrigger
Use this call to query at what value the fee cache will trigger distribution for a given property. Supply a property ID to filter on a specific ID.

omni_getfeetrigger [ propertyid ]

Returns the amount of fees required in the cache to trigger distribution.

Arguments:
1. propertyid           (number, optional) filter the results on this property id

Result:
[                       (array of JSON objects)
  {
    "propertyid" : nnnnnnn,        (number) the property id
    "feetrigger" : "n.nnnnnnnn",   (string) the amount of fees required to trigger distribution
  },
  ...
]

omni_getfeedistribution
Use this call to obtain specifics about a particular fee distribution. Supply the fee distribution ID as parameter (note: each time a fee distribution is triggered, it is given a unique ID).

omni_getfeedistribution distributionid

Get the details for a fee distribution.

Arguments:
1. distributionid           (number, required) the distribution to obtain details for

Result:
{
  "distributionid" : n,          (number) the distribution id
  "propertyid" : n,              (number) the property id of the distributed tokens
  "block" : n,                   (number) the block the distribution occurred
  "amount" : "n.nnnnnnnn",       (string) the amount that was distributed
  "recipients": [                (array of JSON objects) a list of recipients
    {
      "address" : "address",          (string) the address of the recipient
      "amount" : "n.nnnnnnnn"         (string) the amount of fees received by the recipient
    },
    ...
  ]
}

omni_getfeedistributions
Use this call to list all the fee distributions that have been executed for a given property.

omni_getfeedistributions propertyid

Get the details of all fee distributions for a property.

Arguments:
1. propertyid           (number, required) the property id to retrieve distributions for

Result:
[                       (array of JSON objects)
  {
    "distributionid" : n,          (number) the distribution id
    "propertyid" : n,              (number) the property id of the distributed tokens
    "block" : n,                   (number) the block the distribution occurred
    "amount" : "n.nnnnnnnn",       (string) the amount that was distributed
    "recipients": [                (array of JSON objects) a list of recipients
      {
        "address" : "address",          (string) the address of the recipient
        "amount" : "n.nnnnnnnn"         (string) the amount of fees received by the recipient
      },
      ...
    ]
  },
  ...
]

@dexX7 dexX7 added the consensus label Apr 13, 2016

@dexX7 dexX7 added this to the 0.0.10.1 milestone Apr 13, 2016

@zathras-crypto zathras-crypto force-pushed the zathras-crypto:0.0.10-Z-MetaDExFees branch Apr 19, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Apr 19, 2016

Rebased.

@zathras-crypto zathras-crypto force-pushed the zathras-crypto:0.0.10-Z-MetaDExFees branch Apr 19, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Apr 19, 2016

Fixed broken rebase.

@zathras-crypto zathras-crypto force-pushed the zathras-crypto:0.0.10-Z-MetaDExFees branch May 3, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented May 3, 2016

Conflicts resolved & rebased.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 3, 2016

Conflicts resolved & rebased.

Thanks!

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented May 3, 2016

I tidied up, so I think this is about ready for review.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented May 3, 2016

Note to self, update rpc-api.md

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 3, 2016

Meh.. looks like the reorg patch created new conflicts. Sorry!

zathras-crypto added some commits Mar 2, 2016

Further works toward MetaDEx fees
Note: apologies for the rough commit, I'm fleshing things out as I go so the commit isn't really for something specific, but I'm trying to commit more frequently :)
Switch to integer math for fee handling
Note - probably should have been done this way in the first place :)  Since the result of the division is always whole integers and we always want to round down (to avoid taking more than 0.05% fee) it can be done with simple int64s.

Before change:
2016-03-05 19:45:21 entry #       1= 0000000001:720889:7563605
2016-03-05 19:45:21 entry #       2= 0000000002:626672:275433
2016-03-05 19:45:21 entry #       3= 0000000012:487842:10100000
2016-03-05 19:45:21 entry #       4= 0000000068:627338:111,627342:240
2016-03-05 19:45:21 entry #       5= 0000000069:627336:58658,627338:1002034,627339:1350316

After change:
2016-03-05 19:55:30 entry #       1= 0000000001:720889:7563605
2016-03-05 19:55:30 entry #       2= 0000000002:626672:275433
2016-03-05 19:55:30 entry #       3= 0000000012:487842:10100000
2016-03-05 19:55:30 entry #       4= 0000000068:627338:111,627342:240
2016-03-05 19:55:30 entry #       5= 0000000069:627336:58658,627338:1002034,627339:1350316

No difference to final fee cache result after reparsing testnet.  Thanks for the feedback @dexx
Record trading fee in STOListDB and expose it over RPC.
Note fee is only taken on completed trades, and only one side (the liquidity taker) pays the fee.
Add omni_getfeecache RPC call to get currently cached fee amounts
Note: no params will provide a list of all properties and their cached fee amounts (with empty values filtered).  Pass a property id to obtain the cache for just that specific property.
Remove Dev Omni bypass
Note - should it be made a startup option?  It would make the client out of consensus but enable the tests without recompiling.  Perhaps better way to do the tests?

@zathras-crypto zathras-crypto force-pushed the zathras-crypto:0.0.10-Z-MetaDExFees branch to a571e5c May 3, 2016

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented May 3, 2016

Cleaned up conflicts/rebased.

if (msc_debug_fees) PrintToLog(" Current cached amount %d\n", currentCachedAmount);

// Add new fee and rewrite record
if ((currentCachedAmount > 0) && (amount > INT64_MAX - currentCachedAmount)) {

This comment has been minimized.

@dexX7

dexX7 May 15, 2016

Member

Should be std::numeric_limits<int64_t>::max() instead of INT64_MAX. This needs #include <limits>.

This comment has been minimized.

@zathras-crypto

zathras-crypto May 17, 2016

Thanks :) have updated.

if (msc_debug_fees) PrintToLog("Starting PruneCache for prop %d block %d...\n", propertyId, block);
assert(pdb);

int pruneBlock = block-50;

This comment has been minimized.

@dexX7

dexX7 May 18, 2016

Member

So just to clarify, I haven't run this part: are the fees only 50 blocks reorg-safe?

This comment has been minimized.

@zathras-crypto

zathras-crypto May 22, 2016

Fees are only 50 blocks reorg-safe that's correct - I chose that value as it's the same as the rest of the state (we only keep the last 50 blocks).

If a reorg of more than 50 blocks occurs the client clears the state and reparses from scratch - that would include fees.

This comment has been minimized.

@dexX7

dexX7 May 22, 2016

Member

Ah, this is what I assumed. In this case, I suggest to use MAX_STATE_HISTORY from omnicore.h#L35.

This comment has been minimized.

@zathras-crypto

zathras-crypto May 23, 2016

Ahh good idea, thanks

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 18, 2016

Currently fees are distributed to Omni holders, whether it's main or test tokens. I think it would be better, if both systems are seperated, and Test Omni holders receive test tokens, and actual Omni holders only receive main ecosystem tokens. What do you think?

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented May 22, 2016

Currently fees are distributed to Omni holders, whether it's main or test tokens. I think it would be better, if both systems are seperated, and Test Omni holders receive test tokens, and actual Omni holders only receive main ecosystem tokens. What do you think?

Agreed, good suggestion dude - I've made the amendments here 5abe857

std::string address;
uint8_t ecosystem = 1;
if (0 < params.size()) {
address = ParseAddress(params[0]);

This comment has been minimized.

@dexX7

dexX7 May 22, 2016

Member

Could be updated to ParseAddressOrEmpty, so it's possible to fetch all holders solely based on ecosystem.

This comment has been minimized.

@zathras-crypto

zathras-crypto May 23, 2016

Thinking about it, probably the right way to go would be (examples):

omni_getfeeshare - default, show fee share for wallet addresses in main ecosystem
omni_getfeeshare "" 2 - show fee share for wallet addresses in test ecosystem
omni_getfeeshare "*" 2 - show fee share for all addresses in test ecosystem
omni_getfeeshare "1Address" 1 - show fee share for specific address in main ecosystem

I think that best provides consistency with other calls, would you agree?

This comment has been minimized.

@dexX7

dexX7 May 23, 2016

Member

Yes, sounds good to me! ParseAddressOrEmpty should provide this behavior.

This comment has been minimized.

@zathras-crypto

zathras-crypto May 23, 2016

I found wildcards weren't handled in ParseAddressOrEmpty so worked around it in c40b72b

for (OwnerAddrType::reverse_iterator it = receiversSet.rbegin(); it != receiversSet.rend(); ++it) {
addObj = false;
if (address.empty()) {
if (IsMyAddress(it->second)) {

This comment has been minimized.

@dexX7

dexX7 May 22, 2016

Member

Any particular reason to include only wallet results? In my opinion it would be nicer to show all or allow filtering by address.

This comment has been minimized.

@zathras-crypto

zathras-crypto May 23, 2016

That's what we do previously right? Eg omni_listtransactions will show wallet results by default, and specifics if you ask for them?

{
if (fHelp || params.size() > 2)
throw runtime_error(
"omni_getfeeshare [ address ] [ ecosystem ]\n"

This comment has been minimized.

@dexX7

dexX7 May 22, 2016

Member

Nit: currently we use ( xxx yyy ) for optional arguments.

This comment has been minimized.

@zathras-crypto
@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 23, 2016

Thanks for the updates! Looks very good to me.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 24, 2016

In probably a seperate PR, we should set the testnet activation blocks for the recent features to zero, otherwise they are not available. Using 999999 for regtest is fine, and it allows testing of the activation itself.

@dexX7 dexX7 merged commit c40b72b into OmniLayer:omnicore-0.0.10 May 27, 2016

1 check passed

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

dexX7 added a commit that referenced this pull request May 27, 2016

Merge pull request #359
c40b72b Fix up minor nits and improve omni_getfeeshare param handling (zathras-crypto)
0527904 Remove dev msc bypass left in from testing (zathras-crypto)
5abe857 Pay fees from test property trading to Test Omni holders (zathras-crypto)
93e7a72 Change INT64_MAX to std::numeric_limits (thanks @dexX7) (zathras-crypto)
a571e5c Fix a few remaining TODOs (zathras-crypto)
4d4883d Add rollback for fee history DB in event of reorg (zathras-crypto)
24bb242 Add overflow protection to AddFee (zathras-crypto)
8a3fe7e Fix reorg protection for the fee cache (zathras-crypto)
5164386 Update tests to include fees activation via feature ID 9 (zathras-crypto)
108278c Add fees as feature ID 9 and require activation before fees are taken (zathras-crypto)
2a5671d Remove Dev Omni bypass (zathras-crypto)
fd53175 Apply fees only to non-Omni pairs (zathras-crypto)
6f693ab Latest updates to fees concept (zathras-crypto)
8306576 Commit first part of testing (zathras-crypto)
3283798 Reverse orientation of fee reduction (fee should come from liquidity taker, or "buyer") (zathras-crypto)
6752729 Further works towards prototype for fees (zathras-crypto)
1a1393d Add omni_getfeecache RPC call to get currently cached fee amounts (zathras-crypto)
cddad0d Record trading fee in STOListDB and expose it over RPC. (zathras-crypto)
1de7eb5 Switch to integer math for fee handling (zathras-crypto)
c9c7afd Further works toward MetaDEx fees (zathras-crypto)
0d30374 Commenting somehow got messed up, fix (zathras-crypto)
c3309ae Setup a fee cache and start adding helper functions (zathras-crypto)

@dexX7 dexX7 referenced this pull request May 29, 2016

Closed

v11 - Fees #345

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