Remove optimization from matchOrdersAsync in Exchange wrapper

[dekz]

Description

matchOrdersAsync would modify the rightOrder object passed in. Subsequent uses of this object would result in unexpected behaviour (invalid signature, different orderHash, different orderInfo).

await contractWrappers.exchange.matchOrdersAsync(leftSignedOrder, rightSignedOrder,  matcherAccount);
// rightSignedOrder now has maker/takerAssetData = '0x'
await contractWrappers.exchange.getOrderInfoAsync(rightSignedOrder);
// FILLABLE, 0 since the order hash now differs

With the new ABI Encoder we use internally this optimisation is no longer required as the rightOrder will contain a pointer to the leftOrder assetData.

Thanks @arikan for pointing this out.

Testing instructions

Types of changes

Checklist:

• Prefix PR title with [WIP] if necessary.
• Add tests to cover changes as needed.
• Update documentation as needed.
• Add new entries to the relevant CHANGELOG.jsons.

[coveralls]

Coverage decreased (-0.001%) to 85.443% when pulling 54a1fd8 on bug/exchange-wrapper-matchOrders into 797d7c7 on development.

[coveralls]

Coverage decreased (-0.002%) to 85.441% when pulling 007a2d7 on bug/exchange-wrapper-matchOrders into 797d7c7 on development.

[abandeali1]

@dekz we can actually remove this optimization completely since we're using the new ABIEncoder in #1475 . It's actually cheaper now to not do any additional optimizations.

[LogvinovLeon]

@abandeali1 Does that mean that the order hashes on-chain now are gonna be correct and there will be no false InvalidSignature errors?

[abandeali1]

@LogvinovLeon what was the issue before? People accidentally hashing orders with the assetData fields set to 0? If that was the issue, I'm sure this will help.

[LogvinovLeon]

The issue was that if you submit two orders with asset data:
A (maker) and B (taker) for the left and
B (maker) A (taker) for the right
then everything is OK.
But if you accidentally have made a mistake and have submitted two orders:
A (maker) and B (taker) for the left and
A (maker) B (taker) for the right
then on-chain it becomes:
B (maker) A (taker) for the right
because it's copied from the left. Then we verify the signature and throw an invalid signature error. Even if this error includes additional data (order hash) it's the order hash of the modified order, not the one that was submitted. So the user is confused, what's wrong and starts checking signatures. But they're valid.

[abandeali1]

I see, that unfortunately will continue to be a problem until this logic is removed from the contracts as well.