PM: fundAndApproveSigners() wrapper #263
Conversation
yondonfu
commented
Nov 26, 2018
yondonfu
commented
- Add fundAndApproveSigners() function to wrap funding deposit and penalty escrow and approving signers
- Add test for separation of ticket redeemer and recipient
The wrapper function atomically funds a sender's deposit and penalty escrow and also approves a set of signer addresses.
contracts/pm/ERC20TicketBroker.sol
Outdated
| address[] _signers | ||
| ) | ||
| external | ||
| checkDepositPenaltyEscrowSplit(_depositAmount, _penaltyEscrowAmount) |
There was a problem hiding this comment.
won't this always fail because it checks against msg.value which won't be sufficient in an ERC20 approach?
There was a problem hiding this comment.
Right - we actually don't need to check the deposit/penalty escrow split in ERC20TicketBroker since the contract doesn't need to make sure that the correct value is included in the tx. If the user did not approve a sufficient amount of tokens to be transferred by ERC20TicketBroker to cover both the specified deposit and penalty escrow amounts, the ERC20 transferFrom call will fail.
I removed the modifier from ERC20TicketBroker and moved it from TicketBroker into ETHTicketBroker and LivepeerETHTicketBroker in 00f5dfc since the modifier logic does not necessarily apply to all TicketBroker implementations (i.e. ERC20TicketBroker). However, this does introduce duplication of the modifier in LivepeerETHTicketBroker and ETHTicketBroker. Let me know if you have any thoughts on a better approach here.
There was a problem hiding this comment.
We could just decide to leave the modifier in TicketBroker even though not every broker type needs it, if we choose to prioritize DRY (don't repeat yourself) over maximizing sub-type reuse of super type code.
I'm leaning in that direction.. to sweeten the deal we could rename the modifier to explicitly mention it's using ETH value, to reduce the odds of misuse.
wdyt?
There was a problem hiding this comment.
I don't have any strong feelings here, so I am happy to leave the modifier in TicketBroker and update its name to reflect that it deals with ETH value.
| }) | ||
| }) | ||
|
|
||
| describe("fundAndApproveSigners", () => { |
There was a problem hiding this comment.
should we add tests that ensure events are emitted?
There was a problem hiding this comment.
There are already tests for events emitted from the modifiers and approveSigners - since this function just internally calls those modifiers and approveSigners I did not include tests for events emitted in the interest of avoiding additional duplication. WDYT?
There was a problem hiding this comment.
Well, you did add tests for tracking balances, which is also logic that lives inside the modifiers :) so we're testing one part of the modifiers' behavior but not the other.
I share the sentiment that this is redundant, but find it harder to let go of the inconsistency.
Perhaps we should consider dropping the test for balance tracking?
Or re-discuss how we could design these contracts to achieve more straightforward code reuse that doesn't force us to duplicate a bunch of testing code?
|
@yondonfu notice how we're forced to several identical tests to cover both the ETH broker and the LivepeerETH broker. Perhaps we should further discuss how to get rid of this duplication? |
Pull Request Test Coverage Report for Build 575
💛 - Coveralls |
yondonfu
force-pushed
the
yf/sender-wrapper-function
branch
from
00f5dfc to
c73812c
Compare
