requestNetwork icon indicating copy to clipboard operation
requestNetwork copied to clipboard

feat(smart-contracts): batch conversion

Open olivier7delf opened this issue 2 years ago • 4 comments

Add the feature batch conversion ERC20

  • can pay a batch of conversion ERC20 payments
  • can pay in the same time conversion and non_conversion ERC20 payments: like gnosisSafe
  • Include batch eth payment, even if it is not gonna be used directly because it requires modifications (cf Asana)

Archi: https://miro.com/app/board/uXjVOmOyMjg=/ , please unzoom to see also ERC20 + ETH archi

  • Proposition 1) BatchConversionPayment inherits from BatchPayment(Public), the one developed here.
  • Proposition 2 (aborted) BatchConversionPayment call BatchPayment when needed. But it implies some difficulties with approvals and transfer of tokens between proxies: no developed.)

olivier7delf avatar Jul 04 '22 07:07 olivier7delf

Coverage Status

Coverage remained the same at 88.357% when pulling 843e5e770c2f515c1a897bed8c2021c6c93b5553 on feat/batch-conversion into 1879f746488d072390f4852a0c477de63c9a81be on master.

coveralls avatar Jul 04 '22 07:07 coveralls

Thanks for the review!

Regarding the proxies, I agree it can be an issue if they are updated. But it would increase the cost (and the complexity) to add them to function args.

  • Does it arrive often?
  • How bad is it if they cannot use it?

IMO, it should be very exceptional, a solution could be to deploy a second batchConv proxy. This one would keep the old proxies, then, if a user really wants to batch, he can do it. (even if he has to approve again the tokens) Is this solution enough?

olivier7delf avatar Aug 02 '22 15:08 olivier7delf

@olivier7delf it should not happen very often, but it most likely will at some point, and when that happens there will most likely be issues. Even if we redeploy a second batch contract, the user won't be able to batch pay invoices created for different versions of the same payment network.

I'am not sure it would increase complexity, for example we could:

  • Add a mapping(address => uint256).
  • For each Proxy update the whitelist and map the address to the number associated to the proxy kind.
  • Instead of passing the number associated to the proxy kind as we're doing now, pass the proxy address.

If we do that we know that the addressed passed are trusted proxies that we've added, and which kind they are. Maybe it's something we can do in a V2, so it's not a blocker for now but IMO we need to anticipate this else it will become a pain point in the future.

leoslr avatar Aug 03 '22 09:08 leoslr

It has been discussed with Yoann that in order to get batch conversion ASAP, we keep the current version. If we need to update the proxies, we will deploy another batchConv contract (This means we could need to do 2 batches instead of 1 in this specific case) The next generation of batch contracts will get this possibility to make proxies addresses evolve.

olivier7delf avatar Aug 03 '22 15:08 olivier7delf