gelatodigital / gelato-network

V1 implementation of Gelato Network
https://gelato.network/
MIT License
231 stars 29 forks source link

Post-Audit: Allow self-providers to specify global gas price ceil #202

Closed hilmarx closed 4 years ago

hilmarx commented 4 years ago

@gitpusha

hilmarx commented 4 years ago

The whole point of skipping the providerCanExec for self-providers is that they do not have to whitelist any task spec if they themselves are the users.

Hence gelato can be used in 2 different way.

1) by inputting a 3rd party provider who pays for your tx. This has the downside of you only being able to select the actions that they specified 2) by inputting yourself as a provider. Doing so allows you to execute arbitrary actions on gelato that you dont have to whitelist prior to executions. The downside here is that you have to deposit a certain amount of ETH before your transaction gets executed (checked by isProviderLiquid ).

The problem you mentioned arises as a side effect of not requiring self-providers to whitelist task specs, as we coupled whitelisting task specs with limiting gas prices through the gas price ceil (to only have one state read at execution time).

Hence I think in the current design, there is, currently, no way for self-providers to limit the gas prices executors will use for executions.

A possible solution would be to introduce another state variable called selfProviderGlobalGasCeil, which self-providers can select and which will be compare a global gas price ceil vs the current gelato gas price during providerCanExec.

I will discuss this with @gitpusha, but our current prediction is that most users will choose third party providers, hence we think such a feature will probably be not used that much This would enable setting a global gas price ceil, but not a granular, action specific one

adamkolar commented 4 years ago

Another possible solution is to add an optional taskGasPriceCeil argument to submitTask that is only considered in case of self-providers

gitpusha commented 4 years ago

@adamkolar @hilmarx

Another possible solution is to add an optional taskGasPriceCeil argument to submitTask that is only considered in case of self-providers

Yes, we could do that. Even users of Third Party Providers can use this param. This would allow their Providers to offer more granularity to them, in terms of gas pricing, and at the same time the Providers can still put their Gas Price Ceil cap on the actions to limit their max costs.

For SelfProviders: taskGasPriceCeil is evaluated as-is => a taskGasPriceCeil of 0 means 0.

For Third-Party Providers: taskGasPriceCeil of 0 => defaults to Provider's taskSpecGasPriceCeil taskGasPriceCeil greater than Provider's taskSpecGasPriceCeil => reverts or defaults to Provider's taskSpecGasPriceCeil

I would suggest including gasPriceCeil as a field of the Action struct, so that every Action knows and describes its own gasPriceCeil. This way, we also do not have to change the Task submission APIs.

hilmarx commented 4 years ago

I would suggest including gasPriceCeil as a field of the Action struct

Gas prices are not for individual actions though, they are for the whole task. If you give individual actions different gasPriceCeils, the task will only be executed if the gasPrice of the lowest action is reached, which means we can simply use one gas price ceil per task.

I actually don't like that this would add another field in the sumitTask functions, which based on our current hypothesis of "most users will use 3rd party providers", will mean that most of the time a dummy value of 0 has to be passed.

Also, self-providers have another way to limit their spending, by staking only what they are willing to pay. If they stake only $10 worth of ETH and have 2 tasks scheduled, they will indirectly limit themselves to a certain gas price. This of course will not hold true to someone who has e.g. 100 tasks scheduled and has 10 ETH deposited on gelato.

However, from a design perspective, I dislike the fact that self-providers do not have the option to limit their gas costs. As @adamkolar mentioned, it is a design flaw. So I would be fine with passing the ceiling as a parameter to the submitTask functions, which will get stored within an individual task (added to actions and conditions). For self providers, this value will be used. For non.self providers, the value will simply be dismissed and the providers value will be used

gitpusha commented 4 years ago

Yes. We should keep it simple and have 1 gasPrice per Task. My suggestion was meant to leave the submission API clean, as I think passing placeholder values to functions is a bit dirty.

Obviously we can include the gasPriceCeil field on the Task level too, instead of at the Action level.

This way we can leave the submission APIs as they currently are and instead let Providers submit self-describing Tasks.

This would also make it less cumbersome for Third Party Providers that use our code libraries, as they won't have to use any value for Task.gasPriceCeil as we can have this prefilled and default to 0 for them.

gitpusha commented 4 years ago

Decision with @hilmarx on phone:

Include it in Task obj and not in API.

Include checks that use it for Self-Providers.

Ignore it for third party providers.

Side Effect: Might lead to nonsensical values being found in the Tasks of third party providers.

gitpusha commented 4 years ago

Implemented in #213 with commit 372cc31bada1dfb2d0457fa344feaeedb95ba722