WIP: Extract and test Format_MP, add defaults

[dexX7]

• Renamed FormatDivisibleMP to FormatDivisibleAmount
• Renamed FormatIndivisibleMP to FormatIndivisibleAmount
• Added FormatTokenAmount
• Format_Amount is now in namespace mastercore
• Format_Amount is available via mastercore_parse_string.h
• Old behavior not changed, but enhanced

FormatDivisibleAmount(int64_t n) and FormatIndivisibleAmount(int64_t n) without second parameter convert and format a number to string and add minus sign for negative numbers. If there is a use-case to enforce or suppress a sign (positive or negative), the second parameter can be used.

FormatTokenAmount(int64_t n) and FormatTokenAmount(int64_t n, bool divisible) add a minus sign for negative numbers and no sign for positive numbers. Per default (no second paramter provided) the result represents an indivisible amount.

There are quite a few lines which can be simplified, such as:

std::string strAmount;

if (divisible)
    strAmount = FormatDivisibleMP(nTokens);
else
    strAmount = FormatIndivisibleMP(nTokens);

To:

std::string strAmount = FormatTokenAmount(nTokens, divisible);

[m21]

thanks Dexx @zathras-crypto dude mostly your formatting code, comments if any please.

[dexX7]

I would like to add FormatMP here as well, but I don't feel comfortable with including mastercore.h, because the main goal of those "seperated" files is to reduce dependencies.

FormatMP uses isPropertyDivisible which is declared in mastercore.h and implemented in mastercore_sp.cpp and accesses _my_sps.

@m21: you told Faiz to not use the in-memory structures and I believe _my_sps is part of this, right? What do you suggest otherwise? To my understanding of best practises and all, the way to go would be to implement a service class where an accessor of _my_sps is injected, but I have a difficult time to think this through in practise.

The other PR with all those IsPropertyId, IsEcosystem may benefit from a clean solution as well. Testing number ranges is one thing, but another is to check IsValidPropertyId, IsCrowdsaleActive, IsCrowdsalePurchase etc. which would all benefit from access to the "system state", but again, I don't feel comfortable with "polluting" the rather independent files. Ideas welcomed.

[m21]

My comment on in-memory structures applies to historic information. Generally all in-memory structures are correct for that block height, nothing more. (It's a philosophical debate at times: I claim that pure block-explorer functionality does not belong in either Core -- it's a major undertaking, BlockChain.info level, which even Omni hasn't achieved -- there isn't much insight/historical data you can get out of Bitrcoin Core itself for instance).

[dexX7]

Given that every Mastercoin transaction is required to reconstruct the latest state, I don't see a way how you'd get around this - unless intermediate data is immediately discarded.

[dexX7]

@m21: Should I rebase or close?

[ghost]

This looks like a great update, if only for the tests alone.

Needs rebase

[dexX7]

Unfortunally, testing anything of mastercore.cpp/.h is currently basically impossible due to the many (partially missing) cross references and includes, which is one of the reasons why I started to extract some functions.

This PR is difficult to keep in sync, but I'll rebase, if needed.

[zathras-crypto]

Sure, like the idea, looks good. Please rebase I'll get to it quicker next time I promise :)

FYI I explictly stayed away from using a FormatToken which included the divisible lookup within the function itself because if used in loops would end up checking the divisibility repeatedly n times instead of just once at the start. This seems to provide both options so as long as people use it the right way (ie check divisibility and use explicit divisible/indivisible Format functions inside loops where divisibility is fixed) I'm happy :)

Thanks Z

[dexX7]

FYI I explictly stayed away from using a FormatToken which included the divisible lookup within the function itself because if used in loops would end up checking the divisibility repeatedly n times instead of just once at the start.

I wouldn't be surprised, if this gets optimized anyway. In general I tend to prefer simplicity over performance, especially in the dimensions we're talking about.

Right now this PR includes:

// Formats a value as divisible token amount with 8 digits.
// Per default a minus sign is prepended, if n is negative.
// A positive or negative sign can be enforced.
std::string FormatDivisibleAmount(int64_t n);
std::string FormatDivisibleAmount(int64_t n, bool sign);

// Formats a value as indivisible token amount with leading
// minus sign, if n is negative.
std::string FormatIndivisibleAmount(int64_t n);

// Formats a value as token amount and prepends a minus sign,
// if the value is negative. Divisible amounts have 8 digits.
// Per default n is formatted as indivisible amount.
std::string FormatTokenAmount(int64_t n);
std::string FormatTokenAmount(int64_t n, bool divisible);

Few questions @zathras-crypto:

• Is there any case where an explicit "+" prefix is used or would a default behavior of "prepend minus, if negative" be sufficient, especially when it's only available for divisible amounts? Or is there enough justification for extending this, so it's also available for indivisible amounts?
• FormatDivisibleAmount and FormatIndivisibleAmount are sort of redundant in the light of FormatTokenAmount with divisibility parameter. Having more options isn't necessarily bad though. What do you prefer?

Regarding FormatMP and internal divisible lookup, there could also be an overload with CMPSPInfo::Entry, e.g.:

std::string FormatTokenAmount(int64_t n, CMPSPInfo::Entry property);

However, I'm not sure, if this adds much value in practise, because I would assume in many cases this results in getting the Entry, then calling the Format function. A one liner that can be used with a property id only, like FormatMP, seems much more handy.

[zathras-crypto]

Is there any case where an explicit "+" prefix is used or would a default behavior of "prepend minus, if negative" be sufficient, especially when it's only available for divisible amounts? Or is there enough justification for extending this, so it's also available for indivisible amounts?

Well, interestingly I'm actually not convinced of the need for a minus symbol at all at this stage. I don't know of any output (eg RPC) that provides for a negative amount - for example in a trade we would provide amountbought and amountsold, both positive numbers. We don't have any values stored for n that are negative. I'm not against having the capability, I just haven't seen the need for it (yet).

FormatDivisibleAmount and FormatIndivisibleAmount are sort of redundant in the light of FormatTokenAmount with divisibility parameter. Having more options isn't necessarily bad though. What do you prefer?

FormatTokenAmount with divisibility parameter is cleaner.

However, I'm not sure, if this adds much value in practise, be...

The issue I'm trying to address is let's take for example a send to owners and the new getSTO_MP call. Let's say the STO transaction affected 2000 addresses and the getSTO_MP call was made with the "*" filter so we're gonna return them all. That means we're gonna call some kind of format command 2000 times (one for each recipient in the return array). Since we know all those 2000 calls are for the same token thus same divisibility, we don't want to check divisibility within the format call or we end up checking it 2000 times.

I guess in summary:

• I like FormatTokenAmount, nice simplification
• I'm happy with FormatMP, so long as we remember to use an explicit divisibility formatting call (FormatTokenAmount) where appropriate to avoid potential extra work fetching the divsibility

I hope that makes sense :)

[dexX7]

Well, interestingly I'm actually not convinced of the need for a minus symbol at all at this stage.

I just wrapped everything that's out there in other functions, so it's not a new invention or so. Let me ask this way: is there a use case with an explicit "+"? Would simplify things, if not.

FormatTokenAmount with divisibility parameter is cleaner.

So FormatDivisibleAmount and FormatIndivisibleAmount should be dropped?

Since we know all those 2000 calls are ...

Ah. Yeah, sure. My point was more like "probably not often used, given the other functions".

Your summary sounds like a good plan. I'd further change FormatTokenAmount to FormatAmount, so it comes down to:

// Format as indivisible amount
std::string FormatAmount(int64_t n);

// Format as specified
std::string FormatAmount(int64_t n, bool divisible);

// Format based on divisibility of property
std::string FormatAmount(int64_t n, uint32_t property_id);

// Format based on divisibility of property
std::string FormatAmount(int64_t n, CMPSPInfo::Entry property_info);

[zathras-crypto]

Yep agreed sounds good. I'm going to PR the 0.0.9 UI in the next couple of days, so if gonna refactor might be worth waiting for that - boat load of format??divisible functions there.

Thanks :) Z

[m21]

TBD, after the tag waiting for the UI per https://github.com/mastercoin-MSC/mastercore/pull/182#issuecomment-66219938