Add "Send All" transaction type #155

dexX7 · 2015-07-29T23:59:41Z

This PR should pave the way for the new "send all" transaction type.

It resolves #153.

dexX7 · 2015-07-30T00:03:40Z

Completely untested. I have to go now, and just wanted to publish the ongoing progress.

Two points I'm not really sure about:

ideally all sub-sends should be listed via RPC - where do we store them?
given that the transaction type "grant tokens" can trgger crowdsale participation, I think it should be done here, too, though it doesn't seem as easy as calling the simple send logic a few times

zathras-crypto · 2015-07-30T00:05:38Z

src/omnicore/rpctx.cpp

+            "\nArguments:\n"
+            "1. fromaddress          (string, required) the address to send from\n"
+            "2. toaddress            (string, required) the address of the receiver\n"
+            "3. redeemaddress        (string, optional) an address that can spent the transaction dust (sender by default)\n"


nit: spent > spend

Ah, good catch. Should be fixed for the simple send description/API as well.

zathras-crypto · 2015-07-30T00:14:41Z

Awesome stuff :)

ideally all sub-sends should be listed via RPC - where do we store them?

Existing persisted data sets ideally, let me nose around and come back to you...

given that the transaction type "grant tokens" can trgger crowdsale participation, I think it should be done here, too, though it doesn't seem as easy as calling the simple send logic a few times

It's a send to the recipient, so if the recipient has a crowdsale open for one of the tokens sent this does need to trigger crowdsale participation yep. Rather than transferring the tokens directly, is it not feasible to call simple send logic looping each property for the total no of tokens owned by the sender in that prop id?

zathras-crypto · 2015-07-30T00:17:32Z

Perhaps the crowdsale logic should be extracted out of logicMath_SimpleSend so it can be called from other places?

zathras-crypto · 2015-07-30T06:10:19Z

OK, hopefully the stuff I've committed in dexX7#1 will assist in fleshing this out a bit :)

I've addressed a couple of minor problems and added persistent storage of the data and the necessary RPC stuff. Rough around the edges at this early stage but it's functional. Let me know what you think.

Also as some testing, let's start with an empty address:

$ src/omnicore-cli --testnet omni_getallbalancesforaddress mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS
error: {"code":-8,"message":"Address not found"}

Send some tokens in a few different properties to this empty address:

$ src/omnicore-cli --testnet omni_send mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS 1 0.5
07eb5d5b114edcf178d6145158f4d11e45bdd2c1d4e5dfd7db3484f3b82cfbf7
$ src/omnicore-cli --testnet omni_send mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS 12 10
d554de7599cbf598ea182b9ca6e7c0000a22810bc53a55b7bc24026768e85a88
$ src/omnicore-cli --testnet omni_send mpZATHm5ih33ePoSPxFQ4cvWZWaPkN4G5s mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS 13 2
7e2300326bc87b6546493cfc73cb43bf9085c88b12d874f9fc6ba224f054ae8f

Verify that the 'mpZATHqh' address now has a few balances in different properties:

$ src/omnicore-cli --testnet omni_getallbalancesforaddress mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS
[
    {
        "propertyid" : 1,
        "balance" : "0.50000000",
        "reserved" : "0.00000000"
    },
    {
        "propertyid" : 12,
        "balance" : "10.00000000",
        "reserved" : "0.00000000"
    },
    {
        "propertyid" : 13,
        "balance" : "2",
        "reserved" : "0"
    }
]

Let's use another empty address to receive the send all:

$ src/omnicore-cli --testnet omni_getallbalancesforaddress mpZATHofg3kgXc4oQrG6GyJg6J1u8Q5b7Q
error: {"code":-8,"message":"Address not found"}

Execute a send all from the 'mpZATHqh' address to this empty 'mpZATHof' address:

$ src/omnicore-cli --testnet omni_sendall mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS mpZATHofg3kgXc4oQrG6GyJg6J1u8Q5b7Q
10ab5e143a61fbb01c77bed5c89f1497c2d570b8d0f88bb635bd0e57537e9d60

Verify that the sending address ('mpZATHqh') is now empty, and the receiving address ('mpZATHof') now has all the tokens from the sending address:

$ src/omnicore-cli --testnet omni_getallbalancesforaddress mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS
[
    {
        "propertyid" : 1,
        "balance" : "0.00000000",
        "reserved" : "0.00000000"
    },
    {
        "propertyid" : 12,
        "balance" : "0.00000000",
        "reserved" : "0.00000000"
    },
    {
        "propertyid" : 13,
        "balance" : "0",
        "reserved" : "0"
    }
]

$ src/omnicore-cli --testnet omni_getallbalancesforaddress mpZATHofg3kgXc4oQrG6GyJg6J1u8Q5b7Q
[
    {
        "propertyid" : 1,
        "balance" : "0.50000000",
        "reserved" : "0.00000000"
    },
    {
        "propertyid" : 12,
        "balance" : "10.00000000",
        "reserved" : "0.00000000"
    },
    {
        "propertyid" : 13,
        "balance" : "2",
        "reserved" : "0"
    }
]

Now let's query the transaction over RPC to verify retrieval of the details of a send all from persistent storage:

$ src/omnicore-cli --testnet omni_gettransaction 10ab5e143a61fbb01c77bed5c89f1497c2d570b8d0f88bb635bd0e57537e9d60
{
    "txid" : "10ab5e143a61fbb01c77bed5c89f1497c2d570b8d0f88bb635bd0e57537e9d60",
    "sendingaddress" : "mpZATHqhrAd8HpCSvXV7GUKrhc77BD5HXS",
    "referenceaddress" : "mpZATHofg3kgXc4oQrG6GyJg6J1u8Q5b7Q",
    "ismine" : true,
    "confirmations" : 7,
    "fee" : "0.00028221",
    "blocktime" : 1438240459,
    "valid" : true,
    "version" : 0,
    "type_int" : 4,
    "type" : "Send All",
    "subsends" : [
        {
            "propertyid" : 1,
            "divisible" : true,
            "amount" : "0.50000000"
        },
        {
            "propertyid" : 12,
            "divisible" : true,
            "amount" : "10.00000000"
        },
        {
            "propertyid" : 13,
            "divisible" : false,
            "amount" : "2"
        }
    ]
}

And that demonstrates a first draft but functioning send all creation/broadcast and RPC data retrieval.

Thanks dude!!!

dexX7 · 2015-07-30T10:25:09Z

@zathras-crypto: that's a good test. Maybe it's time to get more familiar with OmniJ? ;) Hehe..

Once I find some time, I'm going to add the RPC tests, in a similar manner like this one.

Just to provide some further information: we should also test the behavior, when reserved balances are involved, or crowdsales.

The crowdsale stuff still needs to be managed, and assuming we do, this may be a challenge for the RPC data retrieval (due to the special handling of crowdsale participations).

Rather than transferring the tokens directly, is it not feasible to call simple send logic looping each property for the total no of tokens owned by the sender in that prop id?

This may be slightly messy: we'd have to set the values of the CMPTransaction, then call the simple send logic, update the values, etc. etc. Maybe this is the right thing to do, but I'm not yet really convinced.

Perhaps the crowdsale logic should be extracted out of logicMath_SimpleSend so it can be called from other places?

Seems reasonable, I think. The crowdsale participation could be transformed into a new helper method. If done, then I think it would also make sense to stop calling the simple send logic out of the grant tokens logic, so that each logic method (with helpers, such as crowdsale participation) is self-sustaining.

dexX7 · 2015-07-30T10:32:02Z

Hm.. if the crowdsale participation becomes a helper method of CMPTransaction, then the same issue applies, which is given, if we'd call the simple send logic in a loop: we'd need to update the values over and over again.

marvgmail · 2015-07-30T15:41:29Z

I made some inline comments to the spec PR, see OmniLayer/spec#313

dexX7 · 2015-07-30T16:27:29Z

src/omnicore/omnicore.cpp

+{
+    if (!pdb) return;
+
+    const std::string& key = strprintf("%s-%d", txid.ToString(), subRecordNumber);


Quick note @zathras-crypto: using const T& is great in a context, where the string [...] already exists, say when accessing a value in a map, but in this case simply using [const] std::string key = strprintf(..); is probably to prefer, because we truely want to create a new string, and not point to one. At least this is my understanding.

dexX7 · 2015-08-03T13:36:40Z

Rebased.

I squashed the following commits:

52ada7c Rename getNumberOfPurchases() to getNumberOfSubRecords()
c3471b6 Fix incorrect expectation of element size

And:

7f040ad Add getSendAllDetails() to CMPTxList
c3471b6 Fix incorrect expectation of element size

Since it's to be handled differently, after the consensus params merge, I removed:

5fd41a0 Allow null property for send all in logicMath

... and extended the params/restrictions in e2ed9604d787b6d14be932de88527459de0cdd59.

zathras-crypto · 2015-08-04T05:32:50Z

All looks good to me bud :)

Quick Q - what does "squashing" a commit mean?

dexX7 · 2015-08-04T10:08:36Z

So what should we do with this PR? I'm going to add the ecosystem parameter later, but since the spec discussion is sort of ongoing, we may, or may not merge then.

Quick Q

I combined the commits into one via git rebase -i, see here. :)

zathras-crypto · 2015-08-04T11:36:24Z

So what should we do with this PR? I'm going to add the ecosystem parameter later, but since the spec discussion is sort of ongoing, we may, or may not merge then.

This should remain in progress until spec discussion is final, and then probably go through with adding a feature activation for send all and any final bits prior to merge.

I combined the commits into one via git rebase -i, see here. :)

Ahh nice, thanks!!!

dexX7 · 2015-08-04T12:15:25Z

So just to confirm: we are going to update the PR based on the spec discussion, but hold back the merge, until the spec is finalized?

I pushed the ecosystem support via de3bcd62642b5f6c2e89c453628b74c18b5ec482 and proposed an update for the OmniJ tests with OmniLayer/OmniJ#93.

This recent change will break the tests, until the OmniJ PR is merged. I assume there is going to be some back and forth, as long as the spec isn't finalized.

dexX7 · 2015-08-04T12:20:18Z

@zathras-crypto: how should we handle the entry in the transaction history? I set the property of the CMPTransaction to provide a hint for the UI, which isn't a great workaround, and not fully sufficient:

zathras-crypto · 2015-08-21T06:15:03Z

@zathras-crypto: how should we handle the entry in the transaction history?

Well, since we can't currently display multiple amounts for a transaction I'd suggest we "N/A" Send All types in the short term. Alternatively we could check if just one property ID was sent, and if so use the amount accordingly. I'll grab some time to refocus on this PR, I'd kind of left it until the spec discussion was finalized but that looks stalled for now.

Longer term I'd like histories to have a row entry for each action not necessarily each transaction but I'm not sure how to present that as we don't really have room for a txid column and it's probably more related to the discussion about facilitating multiple actions per transaction. Perhaps even a row per transaction with a little + icon to expand out sub-rows with each action taken, but I'm getting ahead of myself hehe

dexX7 · 2015-08-21T17:25:22Z

Longer term I'd like histories to have a row entry for each action not necessarily each transaction

Yes, this would be awesome!

Well, since we can't currently display multiple amounts for a transaction I'd suggest we "N/A" Send All types in the short term.

Sounds good. This sort of goes hand in hand with what I mentioned in #174 (comment).

We should probably untangle txhistorydialog.cpp#L280-L332, and end up with something like:

std::string DescribeTransactionType(const CMPTransaction& tx)
{
    switch (tx.getType()) {
        case MSC_TYPE_SIMPLE_SEND:
        // ...
    }
    return "Unknown";
}

std::string DescribeAmount(const CMPTransaction& tx)
{
    switch (tx.getType()) {
        case MSC_TYPE_SIMPLE_SEND:
        // ...
    }
    return "N/A";
}

// ... maybe return QString ... etc. ... just some ideas here ...

Alternatively, we could use a transfer object, very similar to HistoryTXObject, but one which doesn't hold std::string etc., and instead the original data types.

The transfer object could then be processed further down the line. In the example above we wouldn't pass around CMPTransaction, but instead HistoryTXObject. I'm not sure, if it makes sense though, or whether it fits into our current approach nicely.

Ideally we'd use something that may be used (at some point) with a clean model/view seperation.

Here is a very good example as starting point, which seems to address a similar problem:

transactiontablemodel.cpp#L280-L597

As first step, instead of passing transfer objects (like HistoryTXObject with original data types), we may just use CMPTransaction directly.

Not really sure here, what do you think?

Note: this is a semantical change only to permit reuse of txlistdb subrecord logic for send all

Fix incorrect expectation of element size

The new transaction format is: [version: 0] [type: 4] [ecosystem]

dexX7 · 2015-08-28T14:49:59Z

Turned out no-token-sends were already invalidated, and I hadn't changed it to begin with.

I rebased the PR, and since we reached consensus regarding the behavior in the context of validity (see #196 (comment)), I'd consider this submission as finalized, at least for now.

We should address the UI parts nevertheless, but I don't consider it as requirement for this PR.

dexX7 · 2015-08-30T19:23:15Z

@zathras-crypto: do you want to add or change anything before we merge this?

zathras-crypto · 2015-08-31T09:19:03Z

@zathras-crypto: do you want to add or change anything before we merge this?

Perhaps just dump in a "quick-fix" for the UI bad amount reporting? Something like this should do the job:

index 2b96740..3175bd6 100644
--- a/src/qt/txhistorydialog.cpp
+++ b/src/qt/txhistorydialog.cpp
@@ -320,7 +320,8 @@ int TXHistoryDialog::PopulateHistoryMap()
             }
         }
         // override - hide display amount for cancels and unknown transactions as we can't display amount/property as no prop exists
-        if (type == MSC_TYPE_METADEX_CANCEL_PRICE || type == MSC_TYPE_METADEX_CANCEL_PAIR || type == MSC_TYPE_METADEX_CANCEL_ECOSYSTEM || htxo.txType == "Unknown") {
+        if (type == MSC_TYPE_METADEX_CANCEL_PRICE || type == MSC_TYPE_METADEX_CANCEL_PAIR ||
+            type == MSC_TYPE_METADEX_CANCEL_ECOSYSTEM || type == MSC_TYPE_SEND_ALL || htxo.txType == "Unknown") {
             displayAmount = "N/A";
         }
         // override - display amount received not STO amount in packet (the total amount) for STOs I didn't send

We should probably untangle...

Yeah I agree this would be good, we could have some functions in omnicore_qtuils for this type of thing - I took a quick look and it should be fairly straight forward.

Ideally we'd use something that may be used (at some point) with a clean model/view seperation

I'm fully open to suggestions and methodology here - I have a very basic understanding of MVC hehe

dexX7 · 2015-08-31T11:46:09Z

Something like this should do the job

Awesome, thanks! I'm going to try it soon. :)

I'm fully open to suggestions and methodology

Yeah, there is a huge post about Model/View programming in QT, but I don't consider it as very feasible to convert everything in the very near future, and to be honest, I haven't really "got" it yet, hehe.

But let me clarify: I'd consider a full blown model with QAbstractTableModel etc. as end goal, and where I was basically going: if we somehow start to untangle the labelling of transaction entries and convert it in a more general form, similar to TXHistoryDialog::shrinkTxType, then we probably already pave the way for this.

zathras-crypto · 2015-08-31T22:14:15Z

Yeah, there is a huge post about Model/View programming in QT

Awesome, that looks like a great post, a little bed-time reading for me hehe :)

if we somehow start to untangle the labelling of transaction entries and convert it in a more general form, similar to TXHistoryDialog::shrinkTxType, then we probably already pave the way for this.

Understood & see what you mean - this should make any future move to MVC easier :)

dexX7 · 2015-09-01T00:29:41Z

I adopted your suggestion in 5d05e9f, and it works fine! :)

Understood & see what you mean

Yeah, the last commit is probably a good example: there were four adjustments, all very similar, and still, they cover only a subset of all transaction types.

It would be an improvement, if we'd have something like QString DescribeAmount().

dexX7 · 2015-09-06T10:07:26Z

@zathras-crypto: would you consider this PR as ready?

zathras-crypto · 2015-09-08T00:19:38Z

@dexX7 - I gave this another quick once over - I think this is ready to go so if you want to merge go right ahead :)

5d05e9f Use "N/A" amount label for "send all" in UI (zathras-crypto) (dexX7) 539cf51 Support ecosystem parameter for "send all" transactions (dexX7) b4d9508 Slightly refine handling of "send all" transactions (dexX7) d6acb32 Fix typo in RPC help, add "omni_sendall" RPC API documentation (dexX7) 2d4a0f0 Add RPC code for send all tx type (zathras-crypto) e549cf9 Add getSendAllDetails() to CMPTxList (zathras-crypto) d910c04 Add omni_sendall to RPC server (zathras-crypto) 7300667 Activate recording of sub sends for send all (zathras-crypto) ae3d8b6 Add recordSendAllSubRecord() to CMPTxList (zathras-crypto) 73e36b1 Record number of sub sends (zathras-crypto) a0c096d Rename getNumberOfPurchases() to getNumberOfSubRecords() (zathras-crypto) 6be9772 Add "send all" transaction type (dexX7)

dexX7 · 2015-09-08T00:29:27Z

@zathras-crypto: awesome, thanks! :)

Speaking of: we probably need to add a new feature identifier to activate "send all".

zathras-crypto · 2015-09-08T00:32:20Z

Speaking of: we probably need to add a new feature identifier to activate "send all".

Hehe yep, good call - I'll add that in

dexX7 · 2015-09-08T00:33:06Z

Thanks!

zathras-crypto reviewed Jul 30, 2015
View reviewed changes

dexX7 mentioned this pull request Jul 30, 2015

send_all_MP method - send all owned omni assets in 1 transaction #153

Closed

dexX7 mentioned this pull request Jul 30, 2015

Added Send All OmniLayer/spec#313

Open

dexX7 reviewed Jul 30, 2015
View reviewed changes

dexX7 force-pushed the oc-0.10-send-all branch 2 times, most recently from 1c62ed0 to 2574bfb Compare July 31, 2015 15:34

dexX7 mentioned this pull request Jul 31, 2015

Add tests for "Send All" transaction type OmniLayer/OmniJ#92

Merged

dexX7 added the status: ready label Jul 31, 2015

dexX7 force-pushed the oc-0.10-send-all branch from 2574bfb to bd04765 Compare August 3, 2015 13:34

dexX7 and others added 5 commits August 28, 2015 16:46

Add "send all" transaction type

6be9772

Rename getNumberOfPurchases() to getNumberOfSubRecords()

a0c096d

Note: this is a semantical change only to permit reuse of txlistdb subrecord logic for send all

Record number of sub sends

73e36b1

Add recordSendAllSubRecord() to CMPTxList

ae3d8b6

Activate recording of sub sends for send all

7300667

zathras-crypto and others added 6 commits August 28, 2015 16:46

Add omni_sendall to RPC server

d910c04

Add getSendAllDetails() to CMPTxList

e549cf9

Fix incorrect expectation of element size

Add RPC code for send all tx type

2d4a0f0

Fix typo in RPC help, add "omni_sendall" RPC API documentation

d6acb32

Slightly refine handling of "send all" transactions

b4d9508

Support ecosystem parameter for "send all" transactions

539cf51

The new transaction format is: [version: 0] [type: 4] [ecosystem]

dexX7 force-pushed the oc-0.10-send-all branch from de3bcd6 to 539cf51 Compare August 28, 2015 14:46

dexX7 force-pushed the oc-0.10-send-all branch from 6808ba6 to 8763c49 Compare September 1, 2015 00:24

Use "N/A" amount label for "send all" in UI (zathras-crypto)

5d05e9f

dexX7 force-pushed the oc-0.10-send-all branch from 8763c49 to 5d05e9f Compare September 1, 2015 00:25

dexX7 merged commit 5d05e9f into OmniLayer:omnicore-0.0.10 Sep 8, 2015

dexX7 modified the milestone: 0.0.10.0 Dec 30, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "Send All" transaction type #155

Add "Send All" transaction type #155

dexX7 commented Jul 29, 2015

dexX7 commented Jul 30, 2015

zathras-crypto Jul 30, 2015

dexX7 Jul 30, 2015

zathras-crypto commented Jul 30, 2015

zathras-crypto commented Jul 30, 2015

zathras-crypto commented Jul 30, 2015

dexX7 commented Jul 30, 2015

dexX7 commented Jul 30, 2015

marvgmail commented Jul 30, 2015

dexX7 Jul 30, 2015

dexX7 commented Aug 3, 2015

zathras-crypto commented Aug 4, 2015

dexX7 commented Aug 4, 2015

zathras-crypto commented Aug 4, 2015

dexX7 commented Aug 4, 2015

dexX7 commented Aug 4, 2015

zathras-crypto commented Aug 21, 2015

dexX7 commented Aug 21, 2015

dexX7 commented Aug 28, 2015

dexX7 commented Aug 30, 2015

zathras-crypto commented Aug 31, 2015

dexX7 commented Aug 31, 2015

zathras-crypto commented Aug 31, 2015

dexX7 commented Sep 1, 2015

dexX7 commented Sep 6, 2015

zathras-crypto commented Sep 8, 2015

dexX7 commented Sep 8, 2015

zathras-crypto commented Sep 8, 2015

dexX7 commented Sep 8, 2015

Add "Send All" transaction type #155

Add "Send All" transaction type #155

Conversation

dexX7 commented Jul 29, 2015

dexX7 commented Jul 30, 2015

zathras-crypto Jul 30, 2015

Choose a reason for hiding this comment

dexX7 Jul 30, 2015

Choose a reason for hiding this comment

zathras-crypto commented Jul 30, 2015

zathras-crypto commented Jul 30, 2015

zathras-crypto commented Jul 30, 2015

dexX7 commented Jul 30, 2015

dexX7 commented Jul 30, 2015

marvgmail commented Jul 30, 2015

dexX7 Jul 30, 2015

Choose a reason for hiding this comment

dexX7 commented Aug 3, 2015

zathras-crypto commented Aug 4, 2015

dexX7 commented Aug 4, 2015

zathras-crypto commented Aug 4, 2015

dexX7 commented Aug 4, 2015

dexX7 commented Aug 4, 2015

zathras-crypto commented Aug 21, 2015

dexX7 commented Aug 21, 2015

dexX7 commented Aug 28, 2015

dexX7 commented Aug 30, 2015

zathras-crypto commented Aug 31, 2015

dexX7 commented Aug 31, 2015

zathras-crypto commented Aug 31, 2015

dexX7 commented Sep 1, 2015

dexX7 commented Sep 6, 2015

zathras-crypto commented Sep 8, 2015

dexX7 commented Sep 8, 2015

zathras-crypto commented Sep 8, 2015

dexX7 commented Sep 8, 2015