[RFC][draft only] add billing token and config #12295
Conversation
|
I see that you haven't updated any README files. Would it make sense to do so? |
shileiwill
changed the title
| // set billing configs | ||
| for (uint256 idx = 0; idx < billingTokens.length; idx++) { | ||
| setBillingConfig(billingTokens[idx], billingConfigs[idx]); | ||
| } |
There was a problem hiding this comment.
Have this logic here in setConfig instead of setConfigTypeSafe to avoid stack too deep issue.
There was a problem hiding this comment.
I think we need this in setConfigTypeSafe() - we'll have to find another way around the stack too deep issues
| @@ -103,6 +103,9 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner { | |||
| mapping(uint256 => bytes) internal s_upkeepOffchainConfig; // general config set by users for each upkeep | |||
| mapping(uint256 => bytes) internal s_upkeepPrivilegeConfig; // general config set by an administrative role for an upkeep | |||
| mapping(address => bytes) internal s_adminPrivilegeConfig; // general config set by an administrative role for an admin | |||
| // billing | |||
| mapping(address => BillingConfig) internal s_billingConfigs; // billing configurations for different tokens | |||
| address[] internal s_billingTokens; // list of billing tokens | |||
There was a problem hiding this comment.
To quickly get the list of billing tokens, I have this address list. would like to hear if there is better solution.
|
Go solidity wrappers are out-of-date, regenerate them via the |
* fix arb gas info vendor and arb module * linting * fix cancel upkeep by owner * fix tests * tidy up cancelUpkeep * update
…error messages when typical configuration is missing (#12298)
* fix: remove unused references for grafana cloud * fix: grafana internal for golangci-lint * chore: other grafana internal credentials
| // set billing configs | ||
| for (uint256 idx = 0; idx < billingTokens.length; idx++) { | ||
| setBillingConfig(billingTokens[idx], billingConfigs[idx]); | ||
| } |
There was a problem hiding this comment.
I think we need this in setConfigTypeSafe() - we'll have to find another way around the stack too deep issues
| @@ -103,6 +103,9 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner { | |||
| mapping(uint256 => bytes) internal s_upkeepOffchainConfig; // general config set by users for each upkeep | |||
| mapping(uint256 => bytes) internal s_upkeepPrivilegeConfig; // general config set by an administrative role for an upkeep | |||
| mapping(address => bytes) internal s_adminPrivilegeConfig; // general config set by an administrative role for an admin | |||
| // billing | |||
| mapping(address => BillingConfig) internal s_billingConfigs; // billing configurations for different tokens | |||
| address[] internal s_billingTokens; // list of billing tokens | |||
| @@ -103,6 +103,9 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner { | |||
| mapping(uint256 => bytes) internal s_upkeepOffchainConfig; // general config set by users for each upkeep | |||
| mapping(uint256 => bytes) internal s_upkeepPrivilegeConfig; // general config set by an administrative role for an upkeep | |||
| mapping(address => bytes) internal s_adminPrivilegeConfig; // general config set by an administrative role for an admin | |||
| // billing | |||
| mapping(address => BillingConfig) internal s_billingConfigs; // billing configurations for different tokens | |||
There was a problem hiding this comment.
I think we're migrating towards named mappings, so something like:
mapping(address billingToken => BillingConfig config) internal s_billingConfigs;| ); | ||
| if (billingTokens.length != billingConfigs.length) revert IncorrectBillingTokenConfigLength(); |
There was a problem hiding this comment.
nitpick, but I think we already have a generic "length" error: ParameterLengthError - could we use this intead?
| for (uint256 idx = 0; idx < billingTokens.length; idx++) { | ||
| setBillingConfig(billingTokens[idx], billingConfigs[idx]); | ||
| } |
There was a problem hiding this comment.
we need to make sure we delete any of the old billing tokens!!
There was a problem hiding this comment.
Before we set new billing configs, we empty the mapping and list first? so every time is new, not append?
| * @param token the address of the token | ||
| * @param config the config for the token | ||
| */ | ||
| function setBillingConfig(address token, BillingConfig memory config) internal { |
There was a problem hiding this comment.
internal / private functions usually start with an underscore :)
There was a problem hiding this comment.
also, all internal functions should live on AutomationRegistryBase :) we try to only put public / external functions on the Root / Logic contracts
| * @param config the config for the token | ||
| */ | ||
| function setBillingConfig(address token, BillingConfig memory config) internal { | ||
| require(token != address(0), "Token address cannot be zero address"); |
There was a problem hiding this comment.
we try to never use error strings, because they take up precious contract space! custom errors are preferred!
| if (!tokenExists(token)) { | ||
| s_billingTokens.push(token); | ||
| } |
There was a problem hiding this comment.
Can we check to see if the BillingConfig exists already instead of doing a nested loop?
| * @param token the address of the token | ||
| * @param config the config for the token | ||
| */ | ||
| function setBillingConfig(address token, BillingConfig memory config) internal { |
There was a problem hiding this comment.
small nitpick, but it might help with the stack too deep error, if we make this function plural, like...
function setBillingConfigs(address[] memory tokens, BillingConfig[] memory configs) {...}and then do the loop inside this function
|
Go solidity wrappers are out-of-date, regenerate them via the |
|
shileiwill
force-pushed
the
AUTO-9111-new
branch
from
a3e161d to
4edeff2
Compare
|
Go solidity wrappers are out-of-date, regenerate them via the |
AUTO-9111
Putting this up to get some early feedback on the structure.
hardhat tests need to be fixed.