Code review results - Other findings

[0xNuggan]

General

Module

I guess he's referring to the comment in OchestratorFactory

ModuleManager

StreamingPaymentProcessor


PaymentClient


OrchestratorFactory

[FHieser]

1. ReadMe -> Instructions not correct
2. Module -> Why init2 functionality
3. Module -> modifier validDependencyData
4. Module -> Use case of dependency data
5. ModuleManager -> Max Module Count
6. StreamingPaymentProcesssor -> unbounded Loops
7. StreamingPaymentProcesssor -> PaymentProcessor Roles
8. PaymentClient -> ensureValidAmount/PaymentOrder
9. PaymentClient -> ensureTokenBalance
10. IproposalFactory -> dependencyData Formatting
11. ProposalFactory -> recommended init form

[FHieser]

in regards of 8: ensureValidAmount is used in inherited contracts (RecurringPaymentManager) so it has an intended purpose. Also Elasticreceipt token has nothing to do with this functionality at all, they just share a similar styling

[FHieser]

in regards of 9: This is correct and seems to be an oversight from us.
The inteded purpose was to pre-shift tokens to the paymentClient Module, so they would be "reserved" for the later payment via the Payment Processor.
This works if all Payment Orders are created simultaneously as the ensureTokenbalance function is a called a single time to update up to the threshold needed to fullfill them. But it wont work in case multiple paymentOrders are created on seperate occasions.
Take the following example:
We create a Payment Order A which has a payment Amount AA.
The PaymentClient will be filled up with tokens until the treshhold of AA is reached
Now we create a PaymentOrder B which has a Payment Amount of BB
AA is bigger than BB so AA>BB
The PAymentClient will try to fill up to the treshhold of BB but it already has AA Tokens in there.
Therefor the amount is already reached accordingly and nothing happens
So the amount of token in the PaymentClient is not AA+BB but just AA

The reason we didnt notice this until now is that the collectPaymentOrders Function of the PaymentClient also calls ensureTokenBalance for all existing PaymentOrders again (in this case it raises the amount to AA+BB), but that is already too late, because we didnt "reserve" the token properly.

My question now is should we keep the reserve Functionality and fix it accordingly (maybe even remove the ensureTokenBalance Function call in the collectPaymentOrder()) or remove it as we currently dont guarantee that the functionality is working properly?
@marvinkruse @saxenism @0xNuggan

[FHieser]

in regards of 9: This is correct and seems to be an oversight from us. The inteded purpose was to pre-shift tokens to the paymentClient Module, so they would be "reserved" for the later payment via the Payment Processor. This works if all Payment Orders are created simultaneously as the ensureTokenbalance function is a called a single time to update up to the threshold needed to fullfill them. But it wont work in case multiple paymentOrders are created on seperate occasions. Take the following example: We create a Payment Order A which has a payment Amount AA. The PaymentClient will be filled up with tokens until the treshhold of AA is reached Now we create a PaymentOrder B which has a Payment Amount of BB AA is bigger than BB so AA>BB The PAymentClient will try to fill up to the treshhold of BB but it already has AA Tokens in there. Therefor the amount is already reached accordingly and nothing happens So the amount of token in the PaymentClient is not AA+BB but just AA

The reason we didnt notice this until now is that the collectPaymentOrders Function of the PaymentClient also calls ensureTokenBalance for all existing PaymentOrders again (in this case it raises the amount to AA+BB), but that is already too late, because we didnt "reserve" the token properly.

My question now is should we keep the reserve Functionality and fix it accordingly (maybe even remove the ensureTokenBalance Function call in the collectPaymentOrder()) or remove it as we currently dont guarantee that the functionality is working properly? @marvinkruse @saxenism @0xNuggan

We actually had a talk about it and we agreed to remove the ensureTokenbalance functionality from the create payment orders altogether.
As it currently stands we immediately call processPayments after we create the PaymentOrders, which removes the time in which "reserves" could be of use anyway.
I will create a new Issue for this

[saxenism]

Cannot remove the validDependencyData from Module.sol since, yes, we do check if the dependencyData is valid or not in the OrchestratorFactory, but what about the cases where modules are added via ModuleManager.addModule() ? That function directly adds a module without going through the OrchestratorFactory.