Change "Range" with the actual variables in the storage structs #310
Closed
andreivladbrg
started this conversation in
General
Replies: 2 comments
-
Thank you for opening this discussion, Andrei. I am sympathetic to your proposal, since 20k gas is non-negligible. Let me sleep more on this idea before we make a final decision - in the short term, we will pass on the contracts to the frontend team as they are now so they can start the refactor of the subgraph. |
Beta Was this translation helpful? Give feedback.
0 replies
-
Locking, as we implemented this in #334. |
Beta Was this translation helpful? Give feedback.
0 replies
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
-
In this PR #220 we faced some issues with the well known: "Stack too deep" error, and because of that we added a couple of structs to get rid of some function parameters. One of them is the
Range
struct:https://github.com/sablierhq/v2-core/blob/55a4d2ed225131247c97dd5ed352c44d755007eb/src/types/DataTypes.sol#L66-L70
Which has the purpose of replacing these variables from create function:
There is an issue with it: @PaulRBerg decided to also use it in the
storage
struct and the gas has increased with 21740 in{SablierV2LockupLinear-createWithRange}
, the time variables are not tight packed anymore.https://github.com/sablierhq/v2-core/blob/55a4d2ed225131247c97dd5ed352c44d755007eb/src/types/DataTypes.sol#L81-L88
Gas comparison
With Range struct
Linear
Pro
Without Range struct
Linear
Pro
My suggestion is to continue using the
Range
struct as a function parameter but to remove it from the storage struct. The downside is that the refactor will take time and I don't know if we can delay more, @razgraf needs the contracts for the app development.Beta Was this translation helpful? Give feedback.
All reactions