Brian/validate order sig #79
Conversation
…hing abilities. Created basic tools for hashing and signing msgs in tests.
| address[4] _addresses, | ||
| uint[5] _values | ||
| ) | ||
| public |
There was a problem hiding this comment.
Public? This seems like something that should be internal or private
There was a problem hiding this comment.
Yeah this one I struggled with, there's no harm in making it public but makes more sense for it to be internal. For expediency purposes I just made it public so that it could be tested. We haven't come up with a framework for testing internal functions I feel like.
There was a problem hiding this comment.
I think these belong in a library of some sort as they don’t alter state. You could then test them by adding a mock contract, kind of how we had one for ExchangeOrderHandler
| uint256 salt; | ||
| bytes32 orderHash; | ||
| } | ||
|
|
There was a problem hiding this comment.
Add some descriptions including what’s at each index of the array params?
There was a problem hiding this comment.
Yeah needed to add javadocs and comments
| ) | ||
| internal | ||
| { | ||
| require(_order.makerTokenAmount > 0 && _order.quantity > 0); |
| uint[5] _values | ||
| ) | ||
| public | ||
| returns(bytes32) |
There was a problem hiding this comment.
Same as in, looks like it should be either internal or belongs to some kind of IssuanceOrder library
| ); | ||
| } | ||
|
|
||
| function generateOrderHash( |
There was a problem hiding this comment.
Same, docs explaining these params
Pull Request Test Coverage Report for Build 376
💛 - Coveralls |
| _order.makerTokenAmount > 0 && _order.quantity > 0, | ||
| INVALID_TOKEN_AMOUNTS | ||
| ); | ||
| // Make sure the order hasn't expired |
There was a problem hiding this comment.
nit: space when you work on this next
|
|
||
| return recAddress == _signerAddress; | ||
| } | ||
|
|
No description provided.