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/add missing fields for transaction retrieval via RPC #173

Merged
merged 2 commits into from Aug 13, 2015

Conversation

Projects
None yet
2 participants
@dexX7
Copy link
Member

dexX7 commented Aug 12, 2015

  • The field labels based on the CMPPending objects were different than the ones generated by the populateRPC__() functions. The field names of the confirmed transaction objects was adopted.
  • Not all stateless fields that can be returned were returned for confirmed transactions.

This PR resolves #170.

dexX7 added some commits Aug 11, 2015

Fix RPC value mismatches of pending transactions
The field names of the confirmed transaction objects was adopted.

@dexX7 dexX7 force-pushed the dexX7:oc-0.10-rpc-gettxs-values branch to 0356f8f Aug 12, 2015

@@ -256,6 +256,7 @@ void populateRPCTypeTradeOffer(CMPTransaction& omniObj, Object& txobj)

// Check levelDB to see if the amount for sale has been amended due to a partial purchase
// TODO: DEx phase 1 really needs an overhaul to work like MetaDEx with original amounts for sale and amounts remaining etc
// TODO: if the amount of the transaction is not used here, then the BTC amount should be recalculated (?)

This comment has been minimized.

Copy link
@zathras-crypto

zathras-crypto Aug 13, 2015

Can you elaborate on this comment mate?

This comment has been minimized.

Copy link
@dexX7

dexX7 Aug 13, 2015

Author Member

Sure:

Alice has only 50 MSC, but offers 1000 MSC for 1000 BTC. The (aweful) way how it's currently handled: the transaction is valid, and the offer is adjusted to 50 MSC for 50 BTC. The adjusted MSC amount is fetched from the DB, but the BTC amount isn't updated:

omni_getbalance moR6aNzFHKSMUdqKb3vDCdetM7pLBHozKY 1
{
  "balance" : "50.00000000",  // <----
  "reserved" : "0.00000000"
}
// DEx Offer of 1000 MSC for 1000 BTC
omni_sendrawtx "moR6aNzFHKSMUdqKb3vDCdetM7pLBHozKY" "0001001400000001000000174876e800000000174876e8000a000000000098968001"
f5c71761e1f6588784720bebf1eecbe3113b99a17d4857ec776352af566293ec
setgenerate true 1
[
  "5a52c577e14918128425d182174cd6b36eaa17d1b92b68604145c5874476cedd"
]
omni_gettransaction f5c71761e1f6588784720bebf1eecbe3113b99a17d4857ec776352af566293ec
{
  "txid": "f5c71761e1f6588784720bebf1eecbe3113b99a17d4857ec776352af566293ec",
  "fee": "0.00010000",
  "sendingaddress": "moR6aNzFHKSMUdqKb3vDCdetM7pLBHozKY",
  "ismine": true,
  "version": 1,
  "type_int": 20,
  "type": "DEx Sell Offer",
  "propertyid": 1,
  "divisible": true,
  "amount": "50.00000000",  // <----
  "bitcoindesired": "1000.00000000",  // <---- !!
  "timelimit": 10,
  "feerequired": "0.10000000",
  "action": "new",
  "valid": true,
  "blockhash": "5a52c577e14918128425d182174cd6b36eaa17d1b92b68604145c5874476cedd",
  "blocktime": 1439467162,
  "block": 103,
  "confirmations": 1
}

This comment has been minimized.

Copy link
@zathras-crypto

zathras-crypto Aug 13, 2015

The (aweful) way how it's currently handled

Ugh, agreed - the source of many a consensus mismatch in the early days when we had multiple implementations :(

but the BTC amount isn't updated

I see, thanks for the explanation - good catch. Perhaps it would be better to handle it just like MetaDEx offers, where the "original amounts" can be provided along with the available (modified) values:

{
  ...
  "amountforsale" : 1000,
  "amountremaining" : 50,
  "amountdesired" : 1000.
  "amounttofill" : 50,
  "unitprice" : "1.00000000"
  ...
}

This comment has been minimized.

Copy link
@dexX7

dexX7 Aug 13, 2015

Author Member

I agree, this would probably be the way to go.

However, there is also a difference between "original amounts" and "the amounts of the transaction".

In the example above the "transaction amounts" are 1000.0/1000.0, while the "original amounts" are 50.0/50.0 (because only that much balance exists), whereby I mean with "original amount" the "amount initially up for sale".

This comment has been minimized.

Copy link
@zathras-crypto

zathras-crypto Aug 13, 2015

Or, you know - we could just invalidate attempts to sell more than your balance hehe :)

This comment has been minimized.

Copy link
@dexX7

dexX7 Aug 13, 2015

Author Member

YES! :)

It's not backwards compatible though, and needs legacy:: handling. There are historical transactions which would otherwise be invalid. (Same goes for two active DEx offers [MSC/TMSC] at the same time, which I checked, when I created #167))

@zathras-crypto

This comment has been minimized.

Copy link

zathras-crypto commented Aug 13, 2015

Reviewed, looks good to me, OK to merge. Also like what you did with properties info :)

@dexX7 dexX7 merged commit 0356f8f into OmniLayer:omnicore-0.0.10 Aug 13, 2015

1 check passed

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

dexX7 added a commit that referenced this pull request Aug 13, 2015

Merge pull request #173
0356f8f Add missing fields for transaction retrieval via RPC (dexX7)
65134df Fix RPC value mismatches of pending transactions (dexX7)

@dexX7 dexX7 modified the milestone: 0.0.10.0 Dec 30, 2015

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.