Fee claiming function

[bh2smith]

Fixes #17

At this moment, we introduce a function transferBalanceTo which accepts a list of token addresses and receiver address. The function then transfers its balance of each listed token to the receiver address. This is, of course, restricted by the "onlyOwner" modifier which remains unimplemented, but will be taken care of in a separate PR via the introduction of an AccessControl contract in #120.

Test Plan

Unit tests.

[fleupold]

As mentioned in the issue, this could be implemented via settle as well, but I think it's nice for convenience to expose this method directly.

[fleupold]

why 33? Seems like an odd value for max line length. Should we use either 79 or 99 as suggested by the solidity style guid

[bh2smith]

I chose 33 because 32 was the default and I needed 33. Thought we could increase it as necessary, but also happy to go with 79.

[nlordell]

I think this should stay 32. This reduces the amount of memory needed to encode the string for the error message to one word (why it was chosen to be 32 in the first place). Revert messages don't have to be descriptive, just unique.

[fleupold]

Oops, I thought this was line length.

[fleupold]

Can we use waffle's mock contracts here instead of real implementations?

[bh2smith]

I guess... but why?

[fleupold]

Because in unit tests it's good practice to use mocks instead of "real objects" as your dependencies. This allows to isolate the behaviour you are trying to test and set clear instructions on what your dependencies are expected to do and how your object under test should react. If you test multiple components together, you are likely writing an integration test.

While for ERC20 it might be easy enough to use a fake implementation to get the desired behavior of our dependencies, this is not the case for more complex dependencies and as such it's more consistent to also mock ERC20 (e.g. as we do in dex-contracts).

[bh2smith]

Isn't it just going be more setup to create a mock contract and tell it what it is supposed to do at every step of the way instead of just having the real thing. Seems to me like the real thing is always better and (in this case) overall shorter to setup. I will look into this in the morning, perhaps I just don't get it.

[nlordell]

One more argument is that we probably want to have tests to make sure this works with non-standard ERC20 implementations (such as transfer reverting vs returning false). If we use the OpenZeppelin implementation for tests, thats not really possible.

[fleupold]

Real objects require future maintainers of the test to understand how the inner logic of the dependency works in order to understand the setup of the test (which in the case of ERC20 can be assumed trivial but in more complicated cases is not). Setting up mocks should be as easy as saying if this method gets called, return this. It can even be as easy as sysing all calls should pass (e.g. return true for ERC20).

[fedgiac]

I think making this function external and storing the array in calldata would make the withdraw transaction cheaper.

[bh2smith]

I can surely make both of these suggestions. In fact I meant to use calldata, but it got changed while fighting with failing testcases.

[nlordell]

Yes, external is better here IMO since we don't ever intend on calling this internally.

[nlordell]

Yes, external is better here IMO since we don't ever intend on calling this internally.

[nlordell]

One more argument is that we probably want to have tests to make sure this works with non-standard ERC20 implementations (such as transfer reverting vs returning false). If we use the OpenZeppelin implementation for tests, thats not really possible.

[bh2smith]

Alright, @nlordell and @fleupold - I have looked into mocking the token contract and landed on the following snippet that just make absolutely no sense to me because I can't see how anything is actually being tested:

const mockTokenTransfer = async (
  token: MockContract,
  sender: string,
  receiver: string,
  amount: BigNumber,
): Promise<void> => {
  const senderCurrentBalance = await token.balanceOf(sender);
  const receiverCurrentBalance = await token.balanceOf(receiver);
  assert(senderCurrentBalance.sub(amount) >= 0);
  await token.mock.balanceOf
    .withArgs(sender)
    .returns(senderCurrentBalance.sub(amount));
  await token.mock.balanceOf
    .withArgs(receiver)
    .returns(receiverCurrentBalance.add(amount));
};



TEST:
  const mintAmount = 1337;

  const token = await waffle.deployMockContract(deployer, ERC20.abi);
  await token.mock.transfer.returns(true);
  await token.mock.balanceOf
    .withArgs(settlement.address)
    .returns(mintAmount);
  await token.mock.balanceOf.withArgs(owner.address).returns(0);

  // Assertions about the balance (which are obviously correct)

  // Perform the operation
  await settlement.transferBalanceTo(
    [token.address],
    owner.address,
  );

  // Update the state of the token after transfer
  await mockTokenTransfer(token, settlement.address, owner.address, mintAmount);

  // Assertions about the balances (again, obviously true)

I'm basically just setting the state however I want it so that everything will always work. Can you please tell me what I don't understand about this?

[fleupold]

@bh2smith you should not need the mockTokenTransfer function (since you would be testing the state you are manually setting in the test). Instead, after performing settlement.transferBalanceTo you could check that.

token.mock.transfer.calls[0] == [amount, receiver]

Which would test that indeed the entirety of balanceOf gets transferred to the ERC.

[bh2smith]

OK then! Yes thanks this is what I was missing. So we only need to test that functions I had expected to be called were called.

[josojo]

ahm... sorry for not reviewing earlier.

But I think we should keep the contract as simple and elegant as possible. Hence, I would not expose another function besides the existing settle function to transfer funds. I think the convenience is not worth it.

Also, withdraws will be very easy to identify later, as they will just send funds to the address of the dao.

[bh2smith]

@josojo - does this mean that task #17 is no longer relevant?

[josojo]

I don't wanna make the call.

I wanna know the opinion of others. But I think we should not have this function ...

I also don't like the modifier "onlyOwner". I would prefer an "onlySolver" modifier for the settle function and not talk about any owners.

[nlordell]

I wanna know the opinion of others. But I think we should not have this function ...

@josojo Do you think this should be implemented via settle?

[bh2smith]

So essentially, once settle is finished with everything it just sends the remaining balances elsewhere?
I thought it would be nice to just collect the funds and only have to claim and distribute them once in a while (gas savings?)

[josojo]

So essentially, once settle is finished with everything it just sends the remaining balance elsewhere?

A solver could also call the settlement function with the only intend to withdraw the funds. As long as the receiving address is the dao, this is legit...

[fedgiac]

In the end, it depends on how much of an overhead it is to call settle instead of a custom function.
A technical difference is that this function can be called only by the owner, wile in principle settle was planned to be callable only by the solvers.
In any case, I don't have a strong opinion either way, we could just wait and see.

[josojo]

A technical difference is that this function can be called only by the owner, wile in principle settle was planned to be callable only by the solvers.

I have hoped for the fact that we don't need an owner for the settlement contract. Maybe the allowance manager, in order to be upgradeable, but not for the settlement contract.

[bh2smith]

I have hoped for the fact that we don't need an owner for the settlement contract. Maybe the allowance manager, in order to be upgradeable, but not for the settlement contract.

As mentioned in the PR description and the "TODO" message in the contract, this is only temporary and the AccessControl will be introduced in a future PR #120.

[fedgiac]

I have hoped for the fact that we don't need an owner for the settlement contract. Maybe the allowance manager, in order to be upgradeable, but not for the settlement contract.

I'm also against any owner in the sense of "somebody who can change the code of the contract," for all contracts involved.
We are going to need an owner in the sense of "somebody (somecontract?) who chooses who the solvers are." The owner referred to here is the second kind, which I doubt we can avoid.

[bh2smith]

I would like to move past the concept of ownership introduced here (only temporarily as a placeholder) as it is not directly relevant to the context of this PR. The plan, moving forward it to rename this modifier to onlyAuthorized and to update the content of this method with a delegate call to an AccessControl contract which will implement the canSolve method and canClaim or as described in issues #103 and #120.

The purpose of this PR is only to implement the feeClaiming functionality.

If it would make things easier, I could rename the modifier to onlyAuthorized here.

[josojo]

We are going to need an owner in the sense of "somebody (somecontract?) who chooses who the solvers are." The owner referred to here is the second kind, which I doubt we can avoid.

I thought we can manage this solvers in an external contract. And yes the external contract can have another owner.

[bh2smith]

Closing in favour of not exposing such a method, since the same functionality can be implemented via "Interactions"