Unification of RPC labels, commands, descriptions

[dexX7]

As continuation of #3 (comment):

@zathras-crypto: I would say for OmniCore at least the API documentation should be the primary doc? It'll need updating if my work gets merged in that's for sure :)

https://github.com/OmniLayer/omnicore/blob/omnicore-0.0.10/doc/apidocumentation.md

I would say for OmniCore at least the API documentation should be the primary doc?

Well, this is the documentation, but I was more thinking about something more compact, and with the intention of finding a naming convention for various places, such as Omni Core, Omni Wallet, OmniJ, ...

Where I was basically going: there are several labels and commands, documentation pieces and more, all referring to the same thing:

• Spec: "Create a Property via Crowdsale with Variable number of Tokens"
• Omni Core RPC command: "sendissuancecrowdsale_OMNI"
• Omni Core RPC description field: "Create Property - Variable"
• Omni Core transaction history: "Create Property"
• Omni Wallet short description: "Create Property - Variable"
• Omni Chest short description: "Create Property - Crowdsale Issuance"
• OmniJ function name: "createCrowdsaleHex" (let's ignore the "Hex" here)
• OmniJ part of a test label: "create crowdsale"
• OmniJ code comment: "create a property via crowdsale with variable supply"

Now this is more or less pretty similar, but when it comes to.. say for example DEx related stuff, there are "trade tokens for bitcoins", "sell bitcoins for mastercoins", "create an offer", and other variations.

It would be nice to have a spreadsheet with:

Type	Command	Label	Description
1	simple_send	"Simple Send"	"Send tokens"
50	create_crowdsale	"Create Crowdsale"	"Create a property via crowdsale with variable supply"
...	...	...	...

In particular, having consistent commands seems useful, for a consistent use of several Omni products. It's not relevant, whether it's "simplesend" or "simple_send", but more of an issue, if one uses "send", but the other "transfer_tokens", especially when it comes to APIs. Hope you see my point? :)

More obvious cases: "property" or "currency"? "tokens" or "coins", ...?

If I find some time, I'm going to create a list for Omni Core, which could then be passed around to collaboratively improve the names and labels.

I mentioned, I dislike "send_MP" and "send_OMNI", but I also have no idea how to make it better - or what others think about this topic in general - so this can basically be considered as trying to find an answer and alternatives.. :)

At some point this can be extended to parameter names, and it would also be useful, if there were a compact list of the fields of RPC results (e.g. of "gettransaction_MP").

[dexX7]

I'm inclined to tag this with a "metadex" label, because the results of gettransaction_MP, gettrade_MP, ... are not consistent.

[zathras-crypto]

To be honest mate I'm not sure I agree here. These are API's we're talking about not end user interfaces and I think it's restrictive to insist that developers of the various different Omni projects must adhere to a specific ruleset for things like command names and parameter structures. This is just my view of course, but I don't really think forcing say OmniJ to adhere to the same structure as OmniCore's RPC layer is necessary, and if anything I think it would hurt - having to do things a specific way can stifle innovation (again just imo).

More obvious cases: "property" or "currency"? "tokens" or "coins", ...?

That I 100% agree with because we're then talking terminology/nomenclature - but I raised this many many moons ago and got unanimous agreement the terms were "property" not "currency" and "tokens" not "coins". I made a fairly aimple argument:

• All currencies are properties but not all properties are currencies - thus "property" is all encapsulating, "currency" is not
• All coins are tokens but not all tokens are coins - thus "tokens" is all encapsulating, "coins" is not

other variations

Yeah it's not that I don't see your point (I do) and project developers where possible should make things easy - but again I think it's just that notion of telling people how they must do things that I'm anti - for example just the same as it would be inappropriate for Bitcoin Core devs to try and tell Coinbase that they should only be using the specific command "sendtoaddress" in their API so it's the same - respectively I think the same for us.

TL:DR; I'm not against us providing some sort of guideline, but I am against us trying to push this on others and I would vote against insisting that other projects adapt the same API structure as OmniCore because I think we would handcuff contributors unnecessarily.

Just my 2 cents of course bud :)

[zathras-crypto]

the results of gettransaction_MP, gettrade_MP, ... are not consistent.

Could you clarify mate, example perhaps?

[dexX7]

I started the issue with an idealistic vision (e.g. normalizing across projects), but to narrow down the scope and to address ...

Could you clarify mate, example perhaps?

... I created a list of labels earlier, which is probably not 100 % up to date, but likely close to: mastercoin-MSC#163 (comment)

In the context of the exchange(s), the following labels are for example very related, but inconsistent:

"amount_desired"
"amount_original"
"amountaccepted"
"amountavailable"
"amountbought"
"amountpaid"
"bitcoindesired"
"currency_desired"
"currency_desired_divisible"
"currency_owned"
"currency_owned_divisible"
"propertyiddesired"
"unit_price"
"unitprice"

It mostly comes down to:

1. underscores vs. no underscores
2. currency vs. property vs. propertyid
3. owned vs. available vs. original

[zathras-crypto]

I started the issue with an idealistic vision

I hope I didn't come across as knocking that :) my view is merely about telling people what to do, if say for example Sean wanted to do OmniJ with a parameter set and output similar to OmniCore's RPC interface I can see the benefit in that - the only thing I was trying to say is that should be up to those respective developers to take the direction that suits their project best so we should try to avoid having things that are supposed to encompass all Omni projects :)

very related, but inconsistent

Ugh, I had not noticed that. I'm going to guess the underscores come in the MetaDEx RPC somewhere? I guess this is an outcome of collaborative development - I think I did all RPC calls except MetaDEx and so they likely follow "my" way which is all lowercase, property not currency, token not coin, no underscores etc. I think Faiz did the MetaDEx RPC calls and so probably applied his own take on field names etc.

Standardizing in our own client is absolutely the right thing to do.

It mostly comes down to:

My vote would be:

1. No underscores
2. Not currency, property or propertyid are both OK though I'd have a slight preference for propertyid simply because that's how I'm used to referring to them (and how they're referred in code)
3. I actually don't like any of those - owned? I'm so used to pulling MetaDEx data straight from the maps I clearly haven't looked at the MetaDEx RPC calls closely enough :( amountforsale, amountdesired, amountremaining, unitprice would be my initial thought.

EDIT: sorry just to clarify, amountforsale and amountdesired would be the amounts specified in the trade (we commonly refer to them as 'original' values but we only do that because we're changing them - since I think we're heading back to making them immutable I don't really see the need to include original in the field name.)

[dexX7]

I hope I didn't come across as knocking that :) my view is merely about telling people what to do ...

Haha, this sounds mean. ;) I never intended to dictate how it should be, but rather wanted point out there are several naming conventions in the wild (and within Omni Core), which would benefit from unification in the long run.

I'm going to guess the underscores come in the MetaDEx RPC somewhere? I think Faiz did the MetaDEx RPC calls and so probably applied his own take on field names etc.

The list I posted in the last comment might also include some labels from the traditional DEx, but I think this is the case.

Back then I extracted the output of getorderbook_MP, gettradessince_MP, gettradehistory_MP, which was similar in all three, and simply moved it into MetaDexObjectToJSON(), so this is likely the primary section to tackle.

I have no preference regarding underscores or no underscores, and I agree that forsale (or leftforsale) and desired sound right. As far as I can see/recall, we usually refer to property instead of currency. It's worth to note though: we should avoid changing existing fields, and ideally limit changes to the new API calls, which are not yet part of a release, to avoid breaking integrations.

[zathras-crypto]

Haha, this sounds mean. ;) I never intended to dictate...

Sorry, poor wording on my part - I know you're not trying to dictate :) Simply put if we want to standardize something it should go in the spec, else it should be up to the respective projects to do as they see fit. I completely agree different naming conventions in OmniCore should be fixed.

It's worth to note though: we should avoid changing existing fields, and ideally limit changes to the new API calls, which are not yet part of a release, to avoid breaking integrations.

I sort of agree - fields in calls that already exist should not be changed, but fields in new API calls I think can be changed prior to release - no-one should be integrating the 0.0.10 branch in production and integrators running it as test integrations have to be open to changes - at least until we hit a sort of 'release candidate' stage so we have the freedom to make new features as best as they possibly can be :)

[dexX7]

I tagged this with "meta dex" due to the required changes of the API to finalize the meta DEx integration, and I'd consider it as resolved, once the meta DEx related changes are in (and further unification might be tackled and tracked later in a seperate, more specific issue).

[dexX7]

@zathras-crypto: gettrade_MP always shows

'amountremaining': '0',
'amountdesired': '0.00000000'

This is because CMPMetaDEx::Set() does not set the remaining amount, which is actually a bit complicated, because via this route (parsing and interpreting a transaction..) the remaining amount isn't dynamically available at all.

[zathras-crypto]

gettrade_MP always shows

Yeah I noticed that too mate. However setAmountRemaining and getAmountRemaining are used in the MetaDEx logic not just on the RPC layer so I figured it was a much deeper problem?

[dexX7]

I'll push the updated logic in a few minutes.

After thinking about this a bit:

• getAmountForSale() should return the original amount for sale (done)
• getAmountDesired() should return the original amount desired (done)
• getAmountRemaining() should return the amount that is still left for sale (broken)

All three should be included in the RPC results imho.

setAmountRemaining() is only used by the core logic, which is a bit mixed up in this context:

• a new order is matched against others
• the other order is cloned at some point
• then the remaining amount is updated
• and the clone-offer is inserted back to the orderbook directly, and not via MetaDEx_INSERT()
• in contrast, the new order is not cloned, but added to the orderbook in MetaDEx_ADD() via MetaDEx_INSERT(), if there are still tokens left for sale

CMPMetaDEx::Set() is a special case, which might not be needed at all, because it is solely used to set values for the optional CMPMetaDEx* in CMPTransaction::logicMath_MetaDEx(), which seems to serve only for the RPC results. Maybe Set() could be removed, and the order might be retrieved via some more straight forward way, e.g. directly from the orderbook/price maps, or via the trade database?

[zathras-crypto]

All three should be included in the RPC results imho.

Yeah they are currently - from populateRPCTransactionObject():

if (MSC_TYPE_METADEX == MPTxTypeInt)
        {
            txobj->push_back(Pair("propertyidforsale", propertyId));
            txobj->push_back(Pair("propertyidforsaleisdivisible", mdex_propertyId_Div));
            txobj->push_back(Pair("amountforsale", FormatMP(propertyId, amount)));
            txobj->push_back(Pair("amountremaining", FormatMP(propertyId, mdex_amountRemaining)));
            txobj->push_back(Pair("propertyiddesired", mdex_propertyWanted));
            txobj->push_back(Pair("propertyiddesiredisdivisible", mdex_propertyWanted_Div));
            txobj->push_back(Pair("amountdesired", FormatMP(mdex_propertyWanted, mdex_amountWanted)));
            txobj->push_back(Pair("unitprice", mdex_unitPriceStr));
            txobj->push_back(Pair("action", mdex_actionStr));
            if (mdex_actionStr == "cancel all") txobj->push_back(Pair("ecosystem",
                isTestEcosystemProperty(propertyId) ? "Test" : "Main"));
        }

I'm still in split minds about amountdesired - I kind of lean in the direction of original value, but then I'm wondering if integrators will struggle to know how much they need to send to buy the complete amountremaining...

CMPMetaDEx::Set() is a special case

Let me take a look at this mate - long story short the 'methodology' for RPC has always been to reparse the original packet simply because we don't store the specifics of a transaction once it's parsed (we only store the outcome) and thus have no way of knowing what was in the original packet without reparsing.

and the order might be retrieved via some more straight forward way, e.g. directly from the orderbook/price maps, or via the trade database

The issue here is kind of as above - once the order is filled it's no longer in the order book, and the trade database contains metrics on the outcome of completed trades not the specifics of the originally listed trades (and these can be different).

To cut a long story short; we only store the outcome of transactions, not transactions themselves (storing transactions themselves is left to the blockchain data).

To be honest, I've always had something in the back of my mind and that is completely changing the way we do transaction retrieval. This would mean storing the specifics of decoded packets in say another db avoiding the need to reparse on the RPC and UI layers to retrieve transaction information. This was all kind of tied in to making parsing more 'atomic' (ie parsing and processing become two seperate stages - a prerequisite for handling unconfirmed txs).

[dexX7]

I kind of lean in the direction of original value, but then I'm wondering if integrators will struggle to know how much they need to send to buy the complete ...

Well, then let's cover all cases and return the original amounts as well as the "amounts to fill" (i.e. remaining amount up for sale and amount still desired, based on that leftover)..

To be honest, I've always had something in the back of my mind and that is completely changing the way we do transaction retrieval.

+1

[dexX7]

Quick heads up:

I started to wrap parsing and checks into functions. There is a good chance, this is totally over the top, but this is basically where it's going:

Value sendchangeissuer_OMNI(const Array& params, bool fHelp)
{
    if (fHelp || params.size() != 3)
        throw runtime_error(
            "sendchangeissuer_OMNI \"fromaddress\" \"toaddress\" propertyid\n"
            "......"
        );

    // obtain parameters & info
    std::string fromAddress = ParseAddress(params[0]);
    std::string toAddress = ParseAddress(params[1]);
    uint32_t propertyId = ParsePropertyId(params[2]);

    RequireExistingProperty(propertyId);
    RequireManagedProperty(propertyId);
    RequireTokenIssuer(fromAddress, propertyId);

    // create a payload for the transaction
    std::vector<unsigned char> payload = CreatePayload_ChangeIssuer(propertyId);

    // request the wallet build the transaction (and if needed commit it)
    return CreateOrSend(fromAddress, toAddress, payload);
}

If something isn't valid, or the requirements are not met, then JSON exceptions are thrown.

[zathras-crypto]

Well, then let's cover all cases and return the original amounts as well as the "amounts to fill" (i.e. remaining amount up for sale and amount still desired, based on that leftover)..

Sorry missed that comment - should we add a field to the metadex class rather than doing calculations at presentation layers (RPC/UI etc)?

Eg we currently have:

amount_forsale - comes from original packet
amount_desired - comes from original packet
amount_remaining - modified according to state

So perhaps we add one more?

amount_neededtofill - modified according to state (amount_remaining * price)

Note neededtofill name doesn't sound quite right but I don't have a better suggestion :)

[dexX7]

Yeah, sounds like a good idea.

I'm also not sure about neededtofill, but I have no ideas either.. stilldesired maybe, but that's even less telling than neededtofill.

[zathras-crypto]

Quick heads up

This seems like a nice code-flow simplification that should make the code more accessible/readable (good thing) :)

[dexX7]

We currently have two "styles" when it comes to the help messages:

Parameters:
FromAddress         : the address to send this transaction from

• PascalCase names
• colon in the middle
• no type information
• optional elements are described in the text (bad example here)
• lower case description

Arguments:
1. propertyid    (int, required) The property identifier

• numbered
• lowercase names
• no colon
• type information given
• optional elements are explicitly tagged as such
• first character upper case in the description

I have no preference regarding the style, as long as:

1. We keep in mind the text is read by humans, not machines (full sentences vs. a few keywords).
2. It's consistent.

When it comes to types, we should probably take a look at the JSON standard, which mentiones number, string, boolean, array, object, null.

[zathras-crypto]

Good point - I prefer (preference only):

• numbered (easily tell where the parameter is supposed to be)
• lowercase names (we use lowercase names in actual RPC output so makes sense to me)
• colon / no colon I'm ambivalent - maybe follow bitcoin-core?
• type information given
• optional elements tagged in the type info
• lower case description (again we don't capitalize anywhere else in RPC output AFAIK)

[dexX7]

Sounds good.

One more point. Let's say that default values should also be given, maybe in this format:

some description (default: 10)

[dexX7]

Ah, and another one:

Usually, examples are given. Do we want to use examples with actual values, or placeholders?

HelpExampleCli("getallbalancesforid_MP", "1")

vs.

HelpExampleCli("getallbalancesforid_MP", "propertyid")

[zathras-crypto]

One more point. Let's say that default values should also be given

+1, any optional param with a default should be noted - happy with the format (default: value)

Usually, examples are given. Do we want to use examples with actual values, or placeholders?

That's a really interesting question actually - I tend to think that placeholders are more descriptive, but values convey a clearer example... I'd probably go example values personally (and that seems to be what bitcoin-core does also)

[dexX7]

I tend to prefer actual values, too.

Regarding the original topic:

The traditional DEx currently has:

"subaction" : "New",
"subaction" : "Update",
"subaction" : "Cancel",

... while MetaDEx uses:

"action" : "new sell",
"action" : "cancel price",
"action" : "cancel pair",
"action" : "cancel all",

I think we should use "new", "update", "cancel", and "cancel xxx" (lower case for both exchanges, no "new sell").

Edit: and "action" instead of "subaction".

Edit: ideally, we find a shorter label, or an alternative, for the following two (MetaDEx):

"propertyidforsale" : 1,
"propertyidforsaleisdivisible" : true,   // <-- long + not id, but property is divisible
"amountforsale" : "0.55000000",
"amountremaining" : "0.00000000",
"propertyiddesired" : 6,
"propertyiddesiredisdivisible" : false,  // <-- long + not id, but property is divisible

[zathras-crypto]

Great minds mate :)

From my RPC branch:

    if (sell_subaction == 1) txobj.push_back(Pair("subaction", "new"));
    if (sell_subaction == 2) txobj.push_back(Pair("subaction", "update"));
    if (sell_subaction == 3) txobj.push_back(Pair("subaction", "cancel"));

and

    if (metaObj.getAction() == 1) actionStr = "new";
    if (metaObj.getAction() == 2) actionStr = "cancel price";
    if (metaObj.getAction() == 3) actionStr = "cancel pair";
    if (metaObj.getAction() == 4) actionStr = "cancel all";

Happy to change subaction to action

[dexX7]

Yes, I think this would be great.

[zathras-crypto]

Yes, I think this would be great.

Nice and easy :) zathras-crypto@803e488

[dexX7]

Almost resolved. There are a few left in rpc.cpp, and I'll take a look at it later.