Skip to content

Commit

Permalink
Do not invalidate MetaDEx cancels that don't have any matches
Browse files Browse the repository at this point in the history
  • Loading branch information
@zathras-crypto
zathras-crypto committed Aug 24, 2015
1 parent 642fb61 commit a751bef
Showing 1 changed file with 25 additions and 14 deletions.
39 changes: 25 additions & 14 deletions src/omnicore/mdex.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,10 +493,7 @@ int mastercore::MetaDEx_CANCEL_AT_PRICE(const uint256& txid, unsigned int block,

if (msc_debug_metadex2) MetaDEx_debug_print();

if (!prices) {

This comment has been minimized.

Sign in to view

Copy link
@dexX7

dexX7 Aug 24, 2015

If I'm not mistaken, then this check also serves as is not NULL protection, and if there are no offers for the given property, then prices would be NULL, as far as I can see:

This comment has been minimized.

Sign in to view

Copy link
@zathras-crypto

zathras-crypto Aug 25, 2015

Author Owner

My bad, I was thinking empty container (begin() == end() in that case) and there would simply be zero iterations of the loop...

Will add them back in...

This comment has been minimized.

Sign in to view

Copy link
@dexX7

dexX7 Aug 25, 2015

It shows we should probably refine the function to make it more robust against mis-use. Maybe something like:

bool mastercore::get_Prices(uint32_t propertyId, md_PricesMap* ppricesMap)
{
    md_PropertiesMap::iterator it = metadex.find(propertyId);
    if (it == metadex.end()) {
        return false;
    }
    if (ppricesMap != NULL) {
        ppricesMap = &(it->second);
    }
    return true;
}
PrintToLog("%s() NOTHING FOUND for %s\n", __FUNCTION__, mdex.ToString());
return rc -1;
}
int numCancelled = 0;

// within the desired property map (given one property) iterate over the items
for (md_PricesMap::iterator my_it = prices->begin(); my_it != prices->end(); ++my_it) {
Expand All @@ -516,7 +513,6 @@ int mastercore::MetaDEx_CANCEL_AT_PRICE(const uint256& txid, unsigned int block,
continue;
}

rc = 0;
PrintToLog("%s(): REMOVING %s\n", __FUNCTION__, p_mdex->ToString());

// move from reserve to main
Expand All @@ -528,10 +524,16 @@ int mastercore::MetaDEx_CANCEL_AT_PRICE(const uint256& txid, unsigned int block,
p_txlistdb->recordMetaDExCancelTX(txid, p_mdex->getHash(), bValid, block, p_mdex->getProperty(), p_mdex->getAmountRemaining());

indexes->erase(iitt++);
numCancelled++;
}
}

if (msc_debug_metadex2) MetaDEx_debug_print();
PrintToLog("%s(): FOUND %d MATCHING TRADES.\n", __FUNCTION__, numCancelled);
rc = 0; // cancels do not require matching trades to be valid

if (numCancelled) {
if (msc_debug_metadex2) MetaDEx_debug_print();
}

return rc;
}
Expand All @@ -546,10 +548,7 @@ int mastercore::MetaDEx_CANCEL_ALL_FOR_PAIR(const uint256& txid, unsigned int bl

if (msc_debug_metadex3) MetaDEx_debug_print();

if (!prices) {

This comment has been minimized.

Sign in to view

Copy link
@dexX7

dexX7 Aug 24, 2015

Same here, see: a751bef#commitcomment-12855913
PrintToLog("%s() NOTHING FOUND\n", __FUNCTION__);
return rc -1;
}
int numCancelled = 0;

// within the desired property map (given one property) iterate over the items
for (md_PricesMap::iterator my_it = prices->begin(); my_it != prices->end(); ++my_it) {
Expand All @@ -565,7 +564,6 @@ int mastercore::MetaDEx_CANCEL_ALL_FOR_PAIR(const uint256& txid, unsigned int bl
continue;
}

rc = 0;
PrintToLog("%s(): REMOVING %s\n", __FUNCTION__, p_mdex->ToString());

// move from reserve to main
Expand All @@ -577,10 +575,16 @@ int mastercore::MetaDEx_CANCEL_ALL_FOR_PAIR(const uint256& txid, unsigned int bl
p_txlistdb->recordMetaDExCancelTX(txid, p_mdex->getHash(), bValid, block, p_mdex->getProperty(), p_mdex->getAmountRemaining());

indexes->erase(iitt++);
numCancelled++;
}
}

if (msc_debug_metadex3) MetaDEx_debug_print();
PrintToLog("%s(): FOUND %d MATCHING TRADES.\n", __FUNCTION__, numCancelled);
rc = 0; // cancels do not require matching trades to be valid

if (numCancelled) {
if (msc_debug_metadex3) MetaDEx_debug_print();
}

return rc;
}
Expand All @@ -596,6 +600,8 @@ int mastercore::MetaDEx_CANCEL_EVERYTHING(const uint256& txid, unsigned int bloc

if (msc_debug_metadex2) MetaDEx_debug_print();

int numCancelled = 0;

PrintToLog("<<<<<<\n");

for (md_PropertiesMap::iterator my_it = metadex.begin(); my_it != metadex.end(); ++my_it) {
Expand All @@ -622,7 +628,6 @@ int mastercore::MetaDEx_CANCEL_EVERYTHING(const uint256& txid, unsigned int bloc
continue;
}

rc = 0;
PrintToLog("%s(): REMOVING %s\n", __FUNCTION__, it->ToString());

// move from reserve to balance
Expand All @@ -634,12 +639,18 @@ int mastercore::MetaDEx_CANCEL_EVERYTHING(const uint256& txid, unsigned int bloc
p_txlistdb->recordMetaDExCancelTX(txid, it->getHash(), bValid, block, it->getProperty(), it->getAmountRemaining());

indexes.erase(it++);
numCancelled++;
}
}
}
PrintToLog(">>>>>>\n");

if (msc_debug_metadex2) MetaDEx_debug_print();
PrintToLog("%s(): FOUND %d MATCHING TRADES.\n", __FUNCTION__, numCancelled);
rc = 0; // cancels do not require matching trades to be valid

if (numCancelled) {
if (msc_debug_metadex2) MetaDEx_debug_print();
}

return rc;
}
Expand Down

1 comment on commit a751bef

@dexX7
Copy link

@dexX7 dexX7 commented on a751bef Aug 24, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NULL checks should be added back, but otherwise ACK. Looks good. Can you create a PR? I'll run the old mastercore-rpc-tests later, which have a bit more tests for the cancel operations (though they only verify the effect on balances, and don't test validity, but more tests are more tests nevertheless).

Please sign in to comment.