Use ABI V2 to shorten settle arg list #1392
Conversation
This allows us to add additional arguments for hash salts and signatures that allow settling on behalf of other users. I kept compatibility with the existing signature and introduced a new settle function using structs. Is this a good idea? Also a better name than `settleChannel2` would be nice. Closes raiden-network#1129
Codecov Report
@@ Coverage Diff @@
## master #1392 +/- ##
==========================================
- Coverage 80.76% 80.43% -0.34%
==========================================
Files 21 21
Lines 1508 1503 -5
Branches 193 183 -10
==========================================
- Hits 1218 1209 -9
- Misses 249 251 +2
- Partials 41 43 +2
Continue to review full report at Codecov.
|
| @@ -124,6 +125,13 @@ contract TokenNetwork is Utils { | |||
| uint256 locked_amount; | |||
| } | |||
|
|
|||
| struct ParticipantSettleInput { | |||
There was a problem hiding this comment.
imho we could go with a shorter name here:
SettleInput (having a mandatory participant argument) should be completely self-explanatory :)
| @@ -692,6 +700,30 @@ contract TokenNetwork is Utils { | |||
| bytes32 participant2_locksroot | |||
| ) | |||
| public | |||
There was a problem hiding this comment.
why wouldn't we change the signature for settleChannel as well?
function settleChannel(
uint256 channel_identifier,
SettleInput participant1,
SettleInput participant2
) ...
There was a problem hiding this comment.
ah, expand-contract pattern...I see.
| @@ -725,16 +759,16 @@ contract TokenNetwork is Utils { | |||
|
|
|||
| require(verifyBalanceHashData( | |||
There was a problem hiding this comment.
verifyBalanceHashData is also never used outside the context of settlement, right? So it could use (Participant)SettleInput as the sole argument.
There was a problem hiding this comment.
I can approve this, as-is.
However, a more thorough refactoring, that makes use of the newly introduced struct in all signatures that share the arguments, would make sense imho.
Unless, of course, this minimal refactoring was done intentionally. In this case I'd like to know the rationale.
This keeps the number of arguments down, as suggested by @konradkonrad.
|
If you can spot any additional refactoring opportunities related to |
This allows us to add additional arguments for hash salts and signatures
that allow settling on behalf of other users.
I kept compatibility with the existing signature and introduced a new
settle function using structs. Is this a good idea? Also a better name
than
settleChannel2would be nice.Closes #1129