Initial work for workflow registry v2: limits #40
Conversation
eutopian
force-pushed
the
DEVSVCS-1014/workflow-registry-limits
branch
7 times, most recently
from
a37a2df to
7a16d99
Compare
| @@ -56,6 +56,33 @@ WorkflowRegistry_registerWorkflow:test_WhenTheWorkflowInputsAreAllValid() (gas: | |||
| WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsInAnAllowedDON() (gas: 936092) | |||
| WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsAnAuthorizedAddress_AndTheWorkflowIsNotInAnAllowedDON() (gas: 510822) | |||
| WorkflowRegistry_requestForceUpdateSecrets:test_WhenTheCallerIsNotAnAuthorizedAddress() (gas: 509176) | |||
| WorkflowRegistry_setDONOverride:test_WhenCallerIsNOTTheOwner() (gas: 120) | |||
There was a problem hiding this comment.
not sure if we should set up a new profile for this and possibly also use pragma 0.8.26 if we do, especially as there will be duplicate functions when it comes to things like register
There was a problem hiding this comment.
For VRF, we set up a different profile for each version to benefit from having a newer Solidity version, so it's certainly possible. In this case, the Foundry profile could be called "workflow_v2".
eutopian
force-pushed
the
DEVSVCS-1014/workflow-registry-limits
branch
3 times, most recently
from
0530b5e to
28b1d42
Compare
| struct Config { | ||
| // Pack three uint32 defaults into a single 96-bit field: | ||
| // Layout in bits: [maxUser(0..31) | maxDON(32..63) | maxUserDON(64..95)] | ||
| uint96 defaultsPacked; |
There was a problem hiding this comment.
Why pack it like this instead of specifying three separate uint32 fields that will also fit into a single storage slot (uint32 = 4 bytes, storage slot size uint256 = 32 bytes, total of 12 bytes in the slot, so you still have some space left)?
There was a problem hiding this comment.
its one less SSTORE when you write. we need to check it often so it saves a little. Even though storage slot is packed together accessing them as separate fields is still separate...
maybe it's not worth it :D
There was a problem hiding this comment.
although... if we have them as separate, we might need separate setters too, otherwise, there's not much gained, and I don't know if this is worth having different setters. otherwise, it's also not worth doing
config.user = a
config.don = b
config.userdon = c each time as that's 3 writes
| if (isSet) { | ||
| s_cfg.userOverride[user] = Config.Value(limit, true); | ||
| } else { | ||
| delete s_cfg.userOverride[user]; |
There was a problem hiding this comment.
Not 100% sure on this, but generally, deleting elements in mappings/arrays is avoided due to gas costs, so resetting values is more desirable. Since you're already using this isSet field value, maybe you can just set it false? In that case, this code is as simple as:
s_cfg.userOverride[user] = Config.Value(limit, isSet);
There was a problem hiding this comment.
delete is only costly for large structures, but this is one slot size, so delete versus setting this to false cost the same storage write but has the slight benefit of the cleared slot refund. the only benefit potentially in allowing it is record I think.
| // Struct to distinguish between unset and explicitly set zero values | ||
| struct Value { | ||
| uint32 value; | ||
| bool isSet; |
There was a problem hiding this comment.
You could also use the uint32 max value or any other arbitrarily large value to have a special meaning, but this is certainly easier to understand!
There was a problem hiding this comment.
This looks like it's more complicated but actually not much more as both are small and fits into one storage slot, but much more explicit and readable than using an arbitrary value to get around the falsey zeros
eutopian
force-pushed
the
DEVSVCS-1014/workflow-registry-limits
branch
from
28b1d42 to
5561750
Compare
No description provided.