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

Fix several bugs in fee system #426

Merged
merged 4 commits into from
Nov 13, 2016

Conversation

zathras-crypto
Copy link

This PR fixes several bugs in the fee system, specifically:

  • Bad data shown at the RPC layer for amounts sold and trading fees charged
  • Incorrect code causing bad application of "Omni-based trades are free" rule (all trades charged a fee)
  • Test script uses OMNI & TOMNI to run the test cases, but these trades should be free.

My own output of the updated test script is below for those who don't want to recompile with dev Omni disabled.

Feedback welcomed :)

Thanks
Z

Preparing a test environment...
   * Starting a fresh regtest daemon
   * Preparing some mature testnet BTC
   * Obtaining a master address to work with
   * Funding the address with some testnet BTC for fees
   * Participating in the Exodus crowdsale to obtain some OMNI
   * Creating an indivisible test property
   * Creating a second indivisible test property
   * Creating a divisible test property
   * Creating a second divisible test property
   * Creating an indivisible test property in the test ecosystem
   * Creating a divisible test property in the test ecosystem
   * Generating addresses to use as fee recipients (OMNI holders)
   * Using a total of 1000 OMNI
   * Seeding mqTy5mTSx9Y8Kx6ED6fajJisVwpuy35ocD with 50.00 OMNI
   * Seeding mk1obB7ry6bhLwF28XW1hh4PY3U4FVgM6h with 100.00 OMNI
   * Seeding myQbFLf2d5Hk1fbRVCRwhSAg7pQn968T3H with 150.00 OMNI
   * Seeding ms6P6kf8X9K9VrH6kUTQzAhSa3joksnS8g with 200.00 OMNI

Activating the fee system...
   * Sending the activation
     # Checking the activation transaction was valid... PASS
   * Mining 10 blocks to forward past the activation block
     # Checking the activation went live as expected... PASS

Checking share of fees for recipients...
   * Checking mqTy5mTSx9Y8Kx6ED6fajJisVwpuy35ocD has a 5 percent share of fees... PASS
   * Checking mk1obB7ry6bhLwF28XW1hh4PY3U4FVgM6h has a 10 percent share of fees... PASS
   * Checking myQbFLf2d5Hk1fbRVCRwhSAg7pQn968T3H has a 15 percent share of fees... PASS
   * Checking ms6P6kf8X9K9VrH6kUTQzAhSa3joksnS8g has a 20 percent share of fees... PASS
   * Checking mwHMAVekCeibdAkXUTJNuJk3ZgpxR6jLQi has a 50 percent share of fees... PASS
   * Checking mwHMAVekCeibdAkXUTJNuJk3ZgpxR6jLQi has a 100 percent share of fees in the test ecosystem... PASS

Testing a trade against self where the first token is OMNI
   * Executing the trade
   * Verifiying the results
      # Checking no fee was taken...
        * Checking the original trade matches to confirm trading fee was 0... PASS
        * Checking the new trade matches to confirm trading fee was 0... PASS
        * Checking the fee cache is empty for property 1... PASS
        * Checking the fee cache is empty for property 3... PASS
        * Checking the trading address didn't lose any #1 tokens after trade... PASS
        * Checking the trading address didn't lose any #3 tokens after trade... PASS

Activating all pair trading...
   * Sending the activation
     # Checking the activation transaction was valid... PASS
   * Mining 10 blocks to forward past the activation block

Testing a trade against self that results in a 1 willet fee for property 3 (1.0 #5 for 2000 #3)
   * Executing the trade
   * Verifiying the results
      # Checking the original trade matches to confirm trading fee was 0... PASS
      # Checking the new trade matches to confirm trading fee was 1... PASS
      # Checking the fee cache now has 1 fee cached for property 3... PASS
      # Checking the trading address now owns 9999999 of property 3... PASS

Testing another trade against self that results in a 5 willet fee for property 3 (1.0 #5 for 10000 #3)
   * Executing the trade
   * Verifiying the results
      # Checking the original trade matches to confirm trading fee was 0... PASS
      # Checking the new trade matches to confirm trading fee was 5... PASS
      # Checking the fee cache now has 6 fee cached for property 3... PASS
      # Checking the trading address now owns 9999994 instead of property 3... PASS

Testing a trade against self that results in a 1 willet fee for property 6 (1.0 #5 for 0.00002 #6)
   * Executing the trade
   * Verifiying the results
      # Checking the original trade matches to confirm trading fee was 0... PASS
      # Checking the new trade matches to confirm trading fee was 0.00000001... PASS
      # Checking the fee cache now has 0.00000001 fee cached for property 6... PASS
      # Checking the trading address now owns 9999.99999999 of property 6... PASS

Testing a trade against self that results in a 5000 willet fee for property 6 (1.0 #5 for 0.1 #6)
   * Executing the trade
   * Verifiying the results
      # Checking the original trade matches to confirm trading fee was 0... PASS
      # Checking the new trade matches to confirm trading fee was 0.00005000... PASS
      # Checking the fee cache now has 0.00005001 fee cached for property 6... PASS
      # Checking the trading address now owns 9999.99994999 of property 6... PASS

Increasing volume to get close to 10000000 fee trigger point for property 6
   * Executing the trades
   * Verifiying the results
      # Checking the fee cache now has 0.09995001 fee cached for property 6... PASS
      # Checking the trading address now owns 9999.90004999 of property 6... PASS

Performing a small trade to take fee cache to 0.1 and trigger distribution for property 6
   * Executing the trade
   * Verifiying the results
      # Checking distribution was triggered and the fee cache is now empty for property 6... PASS
      # Checking mqTy5mTSx9Y8Kx6ED6fajJisVwpuy35ocD received 0.00500000 fee share... PASS
      # Checking mk1obB7ry6bhLwF28XW1hh4PY3U4FVgM6h received 0.01000000 fee share... PASS
      # Checking myQbFLf2d5Hk1fbRVCRwhSAg7pQn968T3H received 0.01500000 fee share... PASS
      # Checking ms6P6kf8X9K9VrH6kUTQzAhSa3joksnS8g received 0.02000000 fee share... PASS
      # Checking mwHMAVekCeibdAkXUTJNuJk3ZgpxR6jLQi received 0.05000000 fee share... PASS

Rolling back the chain to test ability to roll back a distribution during reorg (disconnecting 1 block from tip and mining a replacement)
   * Executing the rollback
   * Clearing the mempool
   * Verifiying the results
      # Checking the block count has been reduced by 1... PASS
   * Mining a replacement block
   * Verifiying the results
      # Checking the block count is the same as before the rollback... PASS
      # Checking the block hash is different from before the rollback... PASS
      # Checking the fee cache now again has 0.09995001 fee cached for property 6... PASS
      # Checking mqTy5mTSx9Y8Kx6ED6fajJisVwpuy35ocD balance has been rolled back to 0... PASS
      # Checking mk1obB7ry6bhLwF28XW1hh4PY3U4FVgM6h balance has been rolled back to 0... PASS
      # Checking myQbFLf2d5Hk1fbRVCRwhSAg7pQn968T3H balance has been rolled back to 0... PASS
      # Checking ms6P6kf8X9K9VrH6kUTQzAhSa3joksnS8g balance has been rolled back to 0... PASS
      # Checking mwHMAVekCeibdAkXUTJNuJk3ZgpxR6jLQi balance has been rolled back to 9999.90004999... PASS
   * Performing a small trade to take fee cache to 0.1 and retrigger distribution for property 6
      # Executing the trade
      # Verifiying the results
        * Checking distribution was triggered again and the fee cache is now empty for property 6... PASS
        * Checking mqTy5mTSx9Y8Kx6ED6fajJisVwpuy35ocD received 0.00500000 fee share... PASS
        * Checking mk1obB7ry6bhLwF28XW1hh4PY3U4FVgM6h received 0.01000000 fee share... PASS
        * Checking myQbFLf2d5Hk1fbRVCRwhSAg7pQn968T3H received 0.01500000 fee share... PASS
        * Checking ms6P6kf8X9K9VrH6kUTQzAhSa3joksnS8g received 0.02000000 fee share... PASS
        * Checking mwHMAVekCeibdAkXUTJNuJk3ZgpxR6jLQi received 0.05000000 fee share... PASS

Rolling back the chain to test ability to roll back a fee cache change during reorg
   # Testing a trade against self that results in a 1 willet fee for property 6 (1.0 #6 for 0.00002 #5)
      * Executing the trade
      * Verifiying the results
         # Checking the fee cache now has 0.00000001 fee cached for property 6... PASS
   # Testing another trade against self that results in a 1 willet fee for property 6 (1.0 #6 for 0.00002 #5)
      * Executing the trade
      * Verifiying the results
         # Checking the fee cache now has 0.00000002 fee cached for property 6... PASS
   # Rolling back the chain to orphan a block (disconnecting 1 block from tip and mining a replacement)
      * Executing the rollback
      * Clearing the mempool
      * Verifiying the results
         # Checking the block count has been reduced by 1... PASS
      * Mining a replacement block
      * Verifiying the results
         # Checking the block count is the same as before the rollback... PASS
         # Checking the block hash is different from before the rollback... PASS
         # Checking the fee cache has been rolled back to 0.00000001 for property 6... PASS

Mining 51 blocks to test that fee cache is not affected by fee pruning
   * Verifiying the results
      # Checking the fee cache is 0.00000001 for property 6... PASS
   * Mining the blocks...
      # Checking the fee cache is still 0.00000001 for property 6... PASS
   * Executing a trade to generate 1 willet fee
      # Checking the fee cache now has 0.00000002 fee cached for property 6... PASS

Adding some test ecosystem volume to trigger distribution
   * Executing the trades
   * Verifiying the results
      # Checking the fee cache now has 90 fee cached for property 2147483651... PASS
      # Checking the trading address now owns 9999910 of property 2147483651... PASS

Triggering distribution in the test ecosystem for property 2147483651
   * Executing the trade
   * Verifiying the results
      # Checking distribution was triggered and the fee cache is now empty for property 2147483651... PASS
      # Checking mwHMAVekCeibdAkXUTJNuJk3ZgpxR6jLQi received 100 fee share... PASS

####################
#  Summary:        #
#    Passed = 67   #
#    Failed = 0    #
####################

Notes:
The following shows a sell for 2000 Indiv for 1.0 Div.

  The original offer
     "matches" : [
         {
             "txid" : "da18e6d80b8340d8f872ff14ff002ab645552760d12060e7e50d62d6a20539ee",
             "block" : 126,
             "address" : "mpwugZU3hfVkQzBumVNAkqB3cJKkpNxotJ",
             "amountsold" : "1999",
             "amountreceived" : "1.00000000",
             "tradingfee" : "0.00000001"
         }
     ],
     "block" : 125,

  The new offer (the liquidity taker):
     "matches" : [
         {
             "txid" : "4812b0389017b479d3fa8a0674d5b7403d07cd6a0d11d4133eb281816a91d7a8",
             "block" : 126,
             "address" : "mpwugZU3hfVkQzBumVNAkqB3cJKkpNxotJ",
             "amountsold" : "1.00000000",
             "amountreceived" : "1999"
         }
     ],
     "block" : 126,

This has the following bugs:
   * Trading fee incorrectly appears in original offer matches when they did not pay any fee
   * Trading fee does not appear in liquidity taking offer matches when they did pay a fee
   * Trading fee shows incorrect divisibility
   * Amount sold attribute in the original offers matches array shows 1999 sold, when in fact 2000 were sold

This commit squashes these issues with an update to getMatchingTrades.  Afterwards the same test gives results:

  The original offer:
     "matches" : [
         {
             "txid" : "612baeb13daf43fa3e9c5e46b736f7b07d1d395c3b2a578befa4060d037c2594",
             "block" : 126,
             "address" : "mta18HMfntLhXHWezu4Nuc4Wzsxw2aDxKv",
             "amountsold" : "2000",
             "amountreceived" : "1.00000000",
             "tradingfee" : "0.00000000"
         }
     ],
     "block" : 125,

The new offer (the liquidity taker):
     "matches" : [
         {
             "txid" : "0214cf43bed73cb6432f9ecc103421ed33ec1c49d2005e01d180b38811f6a87f",
             "block" : 126,
             "address" : "mta18HMfntLhXHWezu4Nuc4Wzsxw2aDxKv",
             "amountsold" : "1.00000000",
             "amountreceived" : "1999",
             "tradingfee" : "1"
         }
     ],
     "block" : 126,
Updates to the test script (will follow later) now check that OMNI pair trades are free.  Prior to this change the results were:

Testing a trade against self where the first token is OMNI
   * Executing the trade
   * Verifiying the results
      # Checking no fee was taken...
        * Checking the original trade matches to confirm trading fee was 0...PASS
        * Checking the new trade matches to confirm trading fee was 0...FAIL (result:1)
        * Checking the fee cache is empty for property 1...PASS
        * Checking the fee cache is empty for property 3...FAIL (result:1)
        * Checking the trading address didn't lose any #1 tokens after trade...PASS
        * Checking the trading address didn't lose any #3 tokens after trade...FAIL (result:9999999)

After this change, the same test provides the following results:

Testing a trade against self where the first token is OMNI
   * Executing the trade
   * Verifiying the results
      # Checking no fee was taken...
        * Checking the original trade matches to confirm trading fee was 0...PASS
        * Checking the new trade matches to confirm trading fee was 0...PASS
        * Checking the fee cache is empty for property 1...PASS
        * Checking the fee cache is empty for property 3...PASS
        * Checking the trading address didn't lose any #1 tokens after trade...PASS
        * Checking the trading address didn't lose any #3 tokens after trade...PASS
Note:
Previous script tested for fees by trading Omni tokens which should have been free.  This updated script checks that Omni based trades are indeed free, activates all pair trading and updates the property handling for the fee tests.
Note: bypass used to run test script only, not appropriate for mainnet
@dexX7 dexX7 added this to the 0.0.11.2 milestone Nov 8, 2016
@dexX7 dexX7 merged commit 1228f7d into OmniLayer:omnicore-0.0.10 Nov 13, 2016
dexX7 added a commit that referenced this pull request Nov 13, 2016
1228f7d Remove devOmni bypass (zathras-crypto)
013d23f Update the fee test script. (zathras-crypto)
b856b29 This commit fixes the evaluation of whether a trade should incur a fee. (zathras-crypto)
cacd1b2 Fix RPC output for trade matches. (zathras-crypto)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants