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

Unlock trading of all pairs on the MetaDEx #361

Merged
merged 5 commits into from May 2, 2016

Conversation

Projects
None yet
2 participants
@zathras-crypto
Copy link

zathras-crypto commented Apr 18, 2016

This PR is intended to add support for activating trading of non-Omni pairs on the distributed exchange.

Feature ID 8 will be used to activate trading all pairs. Prior to activation the current restrictions remain in place.

Note, this should really get merged in before fees ideally, it's a pretty clear cut PR but please shout if you see anything off.

There is a test script included that regtests the activation scenario. Results from my run pasted below.

Preparing a test environment...
   * Starting a fresh regtest daemon with open activation sender
   * 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 divisible test property

Testing a trade against self that uses Omni (1.1 OMNI for 20 #3)
   * Executing the trade
   * Verifiying the results
      # Checking the trade was valid...PASS

Testing a trade against self that doesn't use Omni to confirm non-Omni pairs are disabled (4.45 #4 for 20 #3)
   * Executing the trade
   * Verifiying the results
      # Checking the trade was invalid...PASS

Activating trading on all pairs
   * 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

Testing a trade against self that doesn't use Omni to confirm non-Omni pairs are now enabled (4.45 #4 for 20 #3)
   * Executing the trade
   * Verifiying the results
      # Checking the trade was valid...PASS

####################
#  Summary:        #
#    Passed = 5    #
#    Failed = 0    #
####################
@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Apr 19, 2016

Shouldn't this be activated with the fee stuff?

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

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Apr 19, 2016

@zathras-crypto: just a quick heads up and to refine my position: before merging this, I'll locally do more testing with OmniJ.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Apr 19, 2016

Awesome thanks dude...

In a nutshell before non-Omni pairs we never had indiv/indiv trading so I thought keeping it separate from fees would allow for easier indiv/indiv testing (since fees will skew the test results).

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Apr 19, 2016

Well, in the end it's all int64_t trading, but we'll see. :)

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Apr 20, 2016

Also to note is that the UI still applies trading as if each market is only an OMNI/SPT market.

I'll open a new issue as we need to sort out what we're doing with the UI one way or another :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Apr 24, 2016

While testing the trading of indivisible pairs I noticed that the unit price shown in via RPC isn't intuitive:

$ ./src/omnicore-cli omni_gettrade bfa5b67376ef6e0d8ac3d1847a4ff77a19b5e8ec1e4bedd9b645077ec1580eec
{
    "txid" : "bfa5b67376ef6e0d8ac3d1847a4ff77a19b5e8ec1e4bedd9b645077ec1580eec",
    "fee" : "0.00010000",
    "sendingaddress" : "mt4ZF3rNCUVWKBaVGG1ViUJB2PXPb48Gg3",
    "ismine" : true,
    "version" : 0,
    "type_int" : 25,
    "type" : "MetaDEx trade",
    "propertyidforsale" : 3,
    "propertyidforsaleisdivisible" : false,
    "amountforsale" : "50", // <--------------------------------------------------------
    "propertyiddesired" : 4,
    "propertyiddesiredisdivisible" : false,
    "amountdesired" : "20", // <--------------------------------------------------------
    "unitprice" : "0.00000000400000000000000000000000000000000000000000", // <----------
    "amountremaining" : "50",
    "amounttofill" : "20",
    "status" : "open",
    "matches" : [
    ],
    "valid" : true,
    "blockhash" : "5708509c6925a8a4ea0f69c56f542370221c7f4295b9f676746f8ef491250fa6",
    "blocktime" : 1461494061,
    "block" : 113,
    "confirmations" : 1
}

20 Indivisble are traded for 50 Indivible, whereby I'd assume the unit price should be 0.4 instead of 0.000000004, which is the actual unit price, but shown as divisible representation.

We should refine CMPMetaDEx::displayUnitPrice() and CMPMetaDEx::displayFullUnitPrice(), which currently acts on the assumption that one pair is Omni or TOmni.

This raises the question: are there other similar assumptions, e.g. in the UI?

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Apr 25, 2016

While testing the trading of indivisible pairs I noticed that the unit price shown in via RPC isn't intuitive:

Awesome, thanks for picking that up dude. I've rewritten CMPMetaDEx::displayFullUnitPrice() for a direct unit price and did a quick regtest script to check that each of the types (divisible/indivisible) gives an appropriate response here fcfe287

This raises the question: are there other similar assumptions, e.g. in the UI?

The entire UI experience is based on the notion of OMNI being one side of the pair. We need to do away with the notion of a 'buy' side now otherwise things will start to get confusing (that was doable when all pricing was in OMNI, but I don't think it works for all pair trading). I'm starting to take a look at the trading UI again at the mo.

@dexX7

This comment has been minimized.

Copy link

dexX7 commented on src/omnicore/doc/rpc-api.md in fcfe287 Apr 25, 2016

Should be updated in the RPC help description, too. :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Apr 25, 2016

I really like your tests! :)

I've rewritten CMPMetaDEx::displayFullUnitPrice()

Looks good to me! However, it changes the behavior for Omni-pairs. This isn't necessarily a bad thing, and I prefer the consistency, but we should keep it in mind.

The entire UI experience is based on the notion of OMNI being one side of the pair.

Yes, I'm aware. I was wondering: does non-Omni-pair trading have side effects on the trading interface?

Ideally I'd say we show only xxx/Omni offers in the UI, and filter out xxx/yyy offers without Omni on one side.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Apr 25, 2016

Some CI tests fail, because the unit price is inverted.

edit: I'm going to update them.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Apr 26, 2016

Not sure what happened to my comment dude, sorry

Should be updated in the RPC help description, too. :)

Will do.

Looks good to me! However, it changes the behavior for Omni-pairs. This isn't necessarily a bad thing, and I prefer the consistency, but we should keep it in mind.

Yeah it does, the unit price is always specified in the property desired now. This makes sense to me, in the context of:
"It costs N tokens of the desired property to purchase one unit of the property for sale. Thus N is the unit price."
Does that make sense to you?

We should also add a note to the release notes highlighting the change in behaviour at the RPC layer. I don't think we've got any major players (other than OmniWallet) working on integrating MetaDEx at the moment so it shouldn't be too big a problem I hope.

Yes, I'm aware. I was wondering: does non-Omni-pair trading have side effects on the trading interface?

Ideally I'd say we show only xxx/Omni offers in the UI, and filter out xxx/yyy offers without Omni on one side.

While iterating the order books the UI assumes that if the trade matches the global_metadex_market the other side will be Omni. I started to adapt the old one but it wasn't really working & I've kind of changed quite a lot, but it's given me an opportunity to clean up a bit :)

Some CI tests fail, because the unit price is inverted.

edit: I'm going to update them.

Awesome thanks dude :)

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented Apr 30, 2016

@zathras-crypto: I coded a bunch of tests for the activation and trading, and to me all looks good! :)

You may check out the trading tests to double-check. During the tests exact matches are created and the unit price confirmed. For example: 2 SPX trade for 10 SPY -> unit price should be 10.0, inverted price should be 0.2.

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented May 1, 2016

@zathras-crypto: I coded a bunch of tests for the activation and trading, and to me all looks good! :)

That's awesome, thanks so much dude!

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 1, 2016

What I also noticed: when trading certain pairs, for example 9223372036854775807 (indivisible) for 92233720368.54775806 (divisible), then the unit price isn't shown as x.xxxx..x, but instead as x / y:

{
    ...
    "propertyidforsale" : 5,
    "propertyidforsaleisdivisible" : false,
    "amountforsale" : "9223372036854775807",
    "propertyiddesired" : 6,
    "propertyiddesiredisdivisible" : true,
    "amountdesired" : "92233720368.54775806",
    "unitprice" : "4611686018427387903 / 461168601842738790350000000", // <---
    "amountremaining" : "9223372036854775807",
    "amounttofill" : "92233720368.54775806",
    ...
}

This is due to the range of 64 bit numbers and the potentially higher internal representation of rational_t (see: xToString(const rational_t& value)).

Back then I haven't found a way to deal with it better, but if I recall correctly, I hadn't seen any x / y unit prices in the wild until now.

I wouldn't consider this is a major issue, but something to keep in mind, especially for integrators, which may parse unit prices and expect a certain format.

@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 1, 2016

Locally all tests pass.

Once you updated the RPC help descriptions, and you give your final "go", I'd be happy to merge. :)

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented May 2, 2016

then the unit price isn't shown as x.xxxx..x, but instead as x / y

Hmm - that's a really good catch. Just like you I'm concerned this could break integrations - I'll put this trade on testnet and see how 'Chest copes, but I expect it to fail with the current code base. Should be simple to fix for 'Chest but I wonder what the best way to handle is. Perhaps a note in the release notes? Perhaps extra info in RPC help?

Locally all tests pass.

Awesome :)

Once you updated the RPC help descriptions, and you give your final "go", I'd be happy to merge. :)

Doing the RPC help descriptions now.

@dexX7 dexX7 merged commit 52a68b0 into OmniLayer:omnicore-0.0.10 May 2, 2016

1 check failed

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

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

Merge pull request #361
52a68b0 Update RPC help description for unit price (zathras-crypto)
fcfe287 Fix unit price display at RPC layer (zathras-crypto)
b8a7c9a Add regtest scenario for testing non-Omni trading activation (zathras-crypto)
9b31b80 Add feature-enabled check for non-Omni pair trading restrictions & lift them if activated (zathras-crypto)
5cf2ad6 Add feature ID for trading non-Omni pairs (zathras-crypto)
@dexX7

This comment has been minimized.

Copy link
Member

dexX7 commented May 2, 2016

Alright. :)

Until the testing pull request is merged in OmniJ, Travis CI fails.

dexX7 pushed a commit that referenced this pull request Jan 23, 2017

Squashed 'src/secp256k1/' changes from 6c527ec..8225239
8225239 Merge #433: Make the libcrypto detection fail the newer API.
12de863 Make the libcrypto detection fail the newer API.
2928420 Merge #427: Remove Schnorr from travis as well
8eecc4a Remove Schnorr from travis as well
a8abae7 Merge #310: Add exhaustive test for group functions on a low-order subgroup
b4ceedf Add exhaustive test for verification
83836a9 Add exhaustive tests for group arithmetic, signing, and ecmult on a small group
20b8877 Add exhaustive test for group functions on a low-order subgroup
80773a6 Merge #425: Remove Schnorr experiment
e06e878 Remove Schnorr experiment
04c8ef3 Merge #407: Modify parameter order of internal functions to match API parameter order
6e06696 Merge #411: Remove guarantees about memcmp-ability
40c8d7e Merge #421: Update scalar_4x64_impl.h
a922365 Merge #422: Restructure nonce clearing
3769783 Restructure nonce clearing
0f9e69d Restructure nonce clearing
9d67afa Update scalar_4x64_impl.h
7d15cd7 Merge #413: fix auto-enabled static precompuatation
00c5d2e fix auto-enabled static precompuatation
91219a1 Remove guarantees about memcmp-ability
7a49cac Merge #410: Add string.h include to ecmult_impl
0bbd5d4 Add string.h include to ecmult_impl
353c1bf Fix secp256k1_ge_set_table_gej_var parameter order
541b783 Fix secp256k1_ge_set_all_gej_var parameter order
7d893f4 Fix secp256k1_fe_inv_all_var parameter order
c5b32e1 Merge #405: Make secp256k1_fe_sqrt constant time
926836a Make secp256k1_fe_sqrt constant time
e2a8e92 Merge #404: Replace 3M + 4S doubling formula with 2M + 5S one
8ec49d8 Add note about 2M + 5S doubling formula
5a91bd7 Merge #400: A couple minor cleanups
ac01378 build: add -DSECP256K1_BUILD to benchmark_internal build flags
a6c6f99 Remove a bunch of unused stdlib #includes
65285a6 Merge #403: configure: add flag to disable OpenSSL tests
a9b2a5d configure: add flag to disable OpenSSL tests
b340123 Merge #402: Add support for testing quadratic residues
e6e9805 Add function for testing quadratic residue field/group elements.
efd953a Add Jacobi symbol test via GMP
fa36a0d Merge #401: ecmult_const: unify endomorphism and non-endomorphism skew cases
c6191fd ecmult_const: unify endomorphism and non-endomorphism skew cases
0b3e618 Merge #378: .gitignore build-aux cleanup
6042217 Merge #384: JNI: align shared files copyright/comments to bitcoinj's
24ad20f Merge #399: build: verify that the native compiler works for static precomp
b3be852 Merge #398: Test whether ECDH and Schnorr are enabled for JNI
aa0b1fd build: verify that the native compiler works for static precomp
eee808d Test whether ECDH and Schnorr are enabled for JNI
7b0fb18 Merge #366: ARM assembly implementation of field_10x26 inner (rebase of #173)
001f176 ARM assembly implementation of field_10x26 inner
0172be9 Merge #397: Small fixes for sha256
3f8b78e Fix undefs in hash_impl.h
2ab4695 Fix state size in sha256 struct
6875b01 Merge #386: Add some missing `VERIFY_CHECK(ctx != NULL)`
2c52b5d Merge #389: Cast pointers through uintptr_t under JNI
43097a4 Merge #390: Update bitcoin-core GitHub links
31c9c12 Merge #391: JNI: Only call ecdsa_verify if its inputs parsed correctly
1cb2302 Merge #392: Add testcase which hits additional branch in secp256k1_scalar_sqr
d2ee340 Merge #388: bench_ecdh: fix call to secp256k1_context_create
093a497 Add testcase which hits additional branch in secp256k1_scalar_sqr
a40c701 JNI: Only call ecdsa_verify if its inputs parsed correctly
faa2a11 Update bitcoin-core GitHub links
47b9e78 Cast pointers through uintptr_t under JNI
f36f9c6 bench_ecdh: fix call to secp256k1_context_create
bcc4881 Add some missing `VERIFY_CHECK(ctx != NULL)` for functions that use `ARG_CHECK`
6ceea2c align shared files copyright/comments to bitcoinj's
70141a8 Update .gitignore
7b549b1 Merge #373: build: fix x86_64 asm detection for some compilers
bc7c93c Merge #374: Add note about y=0 being possible on one of the sextic twists
e457018 Merge #364: JNI rebased
86e2d07 JNI library: cleanup, removed unimplemented code
3093576 JNI library
bd2895f Merge pull request #371
e72e93a Add note about y=0 being possible on one of the sextic twists
3f8fdfb build: fix x86_64 asm detection for some compilers
e5a9047 [Trivial] Remove double semicolons
c18b869 Merge pull request #360
3026daa Merge pull request #302
03d4611 Add sage verification script for the group laws
a965937 Merge pull request #361
83221ec Add experimental features to configure
5d4c5a3 Prevent damage_array in the signature test from going out of bounds.
419bf7f Merge pull request #356
03d84a4 Benchmark against OpenSSL verification

git-subtree-dir: src/secp256k1
git-subtree-split: 8225239

@zathras-crypto zathras-crypto deleted the zathras-crypto:0.0.10-Z-MetaDExPhase3 branch Jul 4, 2017

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