Add "Send All" transaction type

[dexX7]

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

It resolves #153.

[dexX7]

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]

nit: spent > spend

[dexX7]

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

[zathras-crypto]

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]

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

[zathras-crypto]

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]

@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]

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]

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

[dexX7]

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]

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]

All looks good to me bud :)

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

[dexX7]

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]

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]

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]

@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]

@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]

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?

[dexX7]

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]

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

[zathras-crypto]

@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]

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]

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]

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]

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

[zathras-crypto]

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

[dexX7]

@zathras-crypto: awesome, thanks! :)

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

[zathras-crypto]

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]

Thanks!