mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Add IsPropertyId, IsTokenAmount, IsSubAction and friends #167

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago

... to avoid magic numbers.

dexX7 commented 9 years ago

Something similar to:

int64_t nAmount = AmountFromValue(params[1]);
int64_t AmountFromValue(const Value& value)
{
    double dAmount = value.get_real();
    if (dAmount <= 0.0 || dAmount > 21000000.0)
        throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
    int64_t nAmount = roundint64(dAmount * COIN);
    if (!MoneyRange(nAmount))
        throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount");
    return nAmount;
}

... might also be an option and would remove a lot of code. What do you think?

m21 commented 9 years ago

Noble goal for sure. I started similar effort(s) with isRangeOK(), various spots where I check against MAX_INT_8_BYTES and alike (don't remember everything). Would be nice to bring it all to elegant completion, and not just in the RPC .cpp file.

dexX7 commented 9 years ago

Ah, I found it in send_INTERNAL_1packet. Hm.. this actually seems redundant. Is there a route where this function is called with faulty values which are not provided by an user via RPC?

dexX7 commented 9 years ago

Updated. Tests can be run via ./test_bitcoin in /src/test. Creating those tests was by the way pretty straight forward. Not even @faizkhan00's regtests made it into 0.0.9.. :/

m21 commented 9 years ago

Need @zathras-crypto and @faizkhan00 to review for sure here.

Dexx can you start using MAX_INT_8_BYTES and alike (define a couple new ones, for MAX INT4 for instance), instead of numbers in the code please?

dexX7 commented 9 years ago

MAX_INT_8_BYTES seems pretty parsing/encoding specific, but I see your point.

How about MAX_NUMBER_OF_COINS (...) instead?

I'm also not sure about the string conversations. Converting to double was chosen, because 1.99999 is definitely not 2 (in the case of ecosystem for example), but on the other hand: something like 23.456 would slip through as allegedly correct property id. It's an improvement to the checks in mastercore_rpc.cpp, but not perfect at the moment.

Last note: IsTokenAmount targets indivisible amounts (which was also based on the mastercore_rpc.cpp checks).

ghost commented 9 years ago

interesting, i support this PR, but you'll need to rebase

also great work on adding boost tests, as well.

dexX7 commented 9 years ago

Updated.

It has functions to check whether numeric or text values equal or are in range of protocol specific values:

There are furthermore constants for specific values, as well as minimum and maximum constants for ranges.

Everything lives within the mastercore namespace.

I removed all updates within mastercore_rpc.cpp and did not replace old defines or references to such values, but I can push another PR to do so, if this iteration of the PR is acceptable.

whitj00 commented 9 years ago

Ut ack

dexX7 commented 9 years ago

This was partially adopted via https://github.com/OmniLayer/omnicore/pull/55.