#176 - Fix dynamic tuple encoding #183
Conversation
…dParameter to resolve incorrect tuple encoding
Codecov Report
@@ Coverage Diff @@
## develop #183 +/- ##
===========================================
+ Coverage 96.64% 96.71% +0.06%
===========================================
Files 303 304 +1
Lines 10012 10278 +266
===========================================
+ Hits 9676 9940 +264
- Misses 336 338 +2
Continue to review full report at Codecov.
|
|
@Biboran thank you, the new implementation looks excellent! Should we try to write tests for such 0x function? It consists of a dynamic collection of dynamic tuples /// @dev Synchronously executes multiple calls of fillOrder.
/// @param orders Array of order specifications.
/// @param takerAssetFillAmounts Array of desired amounts of takerAsset to sell in orders.
/// @param signatures Proofs that orders have been created by makers.
/// @return Amounts filled and fees paid by makers and taker.
/// NOTE: makerAssetFilledAmount and takerAssetFilledAmount may include amounts filled of different assets.
function batchFillOrders(
Order[] memory orders,
uint256[] memory takerAssetFillAmounts,
bytes[] memory signatures
)
public;
returns (FillResults memory totalFillResults) |
I am not sure naming it |
We desperately need more unit tests for ABI encoding. The existing unit tests were very helpful in regression testing. We should implement more ABI encoding unit tests and ganache/infura integration tests. |
|
@Biboran do you have an idea how to make ganache run as a part of the testing pipeline? |
| return heads + tails | ||
| } | ||
|
|
||
| //TODO: headsCount should probably be injected |
There was a problem hiding this comment.
@Biboran in which case we need to inject them? If we want manually construct data?
There was a problem hiding this comment.
headsCount function is not a part of any protocol in this class so it should probably be injected from the outside and removed from this class. For example we could make a separate HeadsCount: IntegerScalar class
I think we can try making a tests runner app for mac os that will spawn ganache and tests Process |
|
Nvmind I found an easy alternative. Lets discuss it in #173 |
|
@Biboran lets merge it and distribute it to CocoaPods. I will spend some time next week to write additional test cases and verify them with |
|
I don't think there is any impact on the README |
As per #176
isDynamicandheadsCountproperties to theABIEncodedParameterto address encoding cases where tuple is dynamicABITupleEncodingclass. I am not sure if it's a good namingAs a side effect of this change you no longer have to worry about unwrapping enveloped tuples as is illustrated in the
testTwoAddressFixedArraysOfTwotest