-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support decimals #12812
support decimals #12812
Conversation
I see you added a changeset file but it does not contain a tag. Please edit the text include at least one of the following tags:
|
Go solidity wrappers are out-of-date, regenerate them via the |
uint256 internal constant ACCOUNTING_FIXED_GAS_OVERHEAD = 51_000; // Fixed overhead per tx | ||
uint256 internal constant ACCOUNTING_PER_UPKEEP_GAS_OVERHEAD = 9_000; // Overhead per upkeep performed in batch | ||
uint256 internal constant ACCOUNTING_FIXED_GAS_OVERHEAD = 51_000 + 30000; // Fixed overhead per tx | ||
uint256 internal constant ACCOUNTING_PER_UPKEEP_GAS_OVERHEAD = 9_000 + 2000; // Overhead per upkeep performed in batch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @RyanRHall : since we modified the transmit path, the gas cost is changed. Without any changes in those constants, the typescript unit tests complains:
4) AutomationRegistry2_3
#transmit
When the upkeep is funded
Gas benchmarking log upkeeps [ @skip-coverage ]
When f=10 calculates gas overhead appropriately within a margin:
Gas overhead calculated is too high, decrease account gas variables (ACCOUNTING_FIXED_GAS_OVERHEAD/ACCOUNTING_PER_SIGNER_GAS_OVERHEAD) by at least 24874
I saw that you also updated those constants in a previous PR, do you recommend updating those numbers based on the unit tests? Appreciate your insights~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we try struct packing differently and then see if we still need to increase the gas overhead? see other comment for struct packing suggestion :)
// if the user can't cover the gas fee, then direct all of the payment to the transmitter and distribute no premium to the DON | ||
payment = balance; | ||
payment18Decimals = balance18Decimals; | ||
// TODO to confirm: both priceUSD and linkUSD use 18 decimals, so here is safe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be confirmed.
uint8 decimals; | ||
// 3rd word only read during cancellation and payment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great case for careful struct packing! Before this PR, only the 1st word is read on the hot path, but now we read the 1st and 3rd words on the hot path (which is probably partly responsible for the increase in gas overhead).
The first word is using 216 (32 + 24 + 160) bits, meaning there are still 40 (256 - 216) bits available! We totally have space in the first word for this field, and by putting it there, we can save an SLOAD :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again, I learnt the power of unit testing. after reordering the struct, gas is under control
@@ -379,7 +379,8 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner { | |||
uint256 fallbackPrice; | |||
// 2nd word only read if stale | |||
uint96 minSpend; | |||
// 3rd word only read during cancellation | |||
uint8 decimals; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the function _setBillingConfig()
can we add a require statement that checks the decimals
value against the decimals()
value returned by the token? This way we can be a little safer against mis-configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, I like the idea.
For the record, as per ERC20 standard, for decimals(), This method can be used to improve usability, but interfaces and other contracts MUST NOT expect these values to be present.
And Openzeppelin used 18 and Etherscan uses 0 when the token doesn’t specify it’s decimals.
So unfortunately I dont think we are not guaranteed to get the decimals from a given token. Accordingly, the Openzepplin IERC20 interface doesnt have decimals().
I guess we will need to rely on the human input to get the decimal places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check to ensure the decimals is in range of [0,24], just trying to avoid human input errors. 99.9% of the tokens should be in this range. let me know if this check is necessary or too noisy.
uint256 internal constant ACCOUNTING_FIXED_GAS_OVERHEAD = 51_000; // Fixed overhead per tx | ||
uint256 internal constant ACCOUNTING_PER_UPKEEP_GAS_OVERHEAD = 9_000; // Overhead per upkeep performed in batch | ||
uint256 internal constant ACCOUNTING_FIXED_GAS_OVERHEAD = 51_000 + 30000; // Fixed overhead per tx | ||
uint256 internal constant ACCOUNTING_PER_UPKEEP_GAS_OVERHEAD = 9_000 + 2000; // Overhead per upkeep performed in batch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we try struct packing differently and then see if we still need to increase the gas overhead? see other comment for struct packing suggestion :)
@@ -975,29 +979,43 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner { | |||
|
|||
PaymentReceipt memory receipt = _calculatePaymentAmount(hotVars, paymentParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the PaymentReceipt
let's say:
gasCharge
/premium
are in the billing token, using the billing token's native decimalsgasReimbursementJuels
/premiumJuels
use LINK, which always uses 18 decimals
// this balance uses the token's decimal places | ||
uint96 balanceXDecimals = upkeep.balance; | ||
// adjust to 18 decimal places to be comparable with the following payment | ||
uint96 balance18Decimals = SafeCast.toUint96( | ||
balanceXDecimals * 10 ** (18 - paymentParams.billingTokenParams.decimals) | ||
); | ||
|
||
// payment always has 18 decimals | ||
uint96 payment18Decimals = receipt.gasCharge + receipt.premium; | ||
|
||
// this shouldn't happen, but in rare edge cases, we charge the full balance in case the user | ||
// can't cover the amount owed | ||
if (balance < receipt.gasCharge) { | ||
if (balance18Decimals < receipt.gasCharge) { | ||
// if the user can't cover the gas fee, then direct all of the payment to the transmitter and distribute no premium to the DON | ||
payment = balance; | ||
payment18Decimals = balance18Decimals; | ||
// TODO to confirm: both priceUSD and linkUSD use 18 decimals, so here is safe? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might get a cleaner solution if we try to handle the decimals problem in _calculatePaymentAmount()
instead of _handlePayment()
(see other comment about PaymentReceipt
). I think the _handlePayment()
code can remain as-is if we just tweak the PaymentReceipt
struct so that it always uses the same decimals as the billing token :)
uint96 balanceXDecimals = upkeep.balance; | ||
// adjust to 18 decimal places to be comparable with the following payment | ||
uint96 balance18Decimals = SafeCast.toUint96( | ||
balanceXDecimals * 10 ** (18 - paymentParams.billingTokenParams.decimals) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will underflow and revert if the billing token uses >18 decimals (no idea how common this is, but I think we might as well support it!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably only need one changeset file 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a foundry test for getMinBalance()
that compares the min balance for tokens with different numbers of decimals? (but all the same other params)? This will test that decimals is being handled correctly by our payment calculation code path.
// most ERC20 tokens are 18 decimals, we support tokens with up to 24 decimals | ||
if (config.decimals > 24) { | ||
revert InvalidToken(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only up to 24?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked several mainnet tokens and found that all of them have a decimals()
function, so I think we can require(billingToken.decimals() == billingConfig.decimals)
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here can we also make a requirement that billindConfig.aggregator.decimals() == 8
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why only up to 24?
24 is just a number, we can change.
Here can we also make a requirement that billindConfig.aggregator.decimals() == 8?
I am pretty confident that this decimals() can be done!
I checked several mainnet tokens and found that all of them have a decimals() function, so I think we can require(billingToken.decimals() == billingConfig.decimals)
Compiler will not be happy with this. our billingToken is of type IERC20 which doesnt have decimals().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, typo in the above message, the billingConfig.aggregator decimals should be 8 not 18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler will not be happy with this. our billingToken is of type IERC20 which doesnt have decimals().
Can we change from IERC20 to something that supports the decimals() function? How about this guy?
We can alias the import so it's a little cleaner...
import {IERC20Metadata as IERC20} from ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will explore this IERC20Metadata.
There is definitely way to get the decimals. If OZ's interfaces dont provide that, we can make a low level call from the token address and fetch the decimals if it exists.
address(token).call(abi.encodeWithSignature("decimals()"))...
@@ -660,6 +663,7 @@ abstract contract AutomationRegistryBase2_3 is ConfirmedOwner { | |||
HotVars memory hotVars, | |||
PaymentParams memory paymentParams | |||
) internal view returns (PaymentReceipt memory receipt) { | |||
uint8 decimals = paymentParams.billingTokenParams.decimals; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's probably cheaper to cast this as a uint256
here (otherwise, the evm has to convert it implicitly every time it is used in a calculation below)
if (decimals < 18) { | ||
receipt.premium = SafeCast.toUint96(premium18Decimals / (10 ** (18 - decimals))); | ||
} else if (decimals > 18) { | ||
receipt.premium = SafeCast.toUint96(premium18Decimals * (10 ** (decimals - 18))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we're doing multiplication (line 696) after division (line 691) I think we're potentially losing some precision. Could we instead do something like this...
uint256 numeratorScalingFactor = decimals > 18 ? 10 ** (decimals - 18) : 1;
uint256 denominatorScalingFactor = decimals < 18 ? 10 ** (18 - decimals) : 1;
receipt.premium = SafeCast.toUint96(
(premiumHexaicosaUSD * numeratorScalingFactor) / (paymentParams.billingTokenParams.priceUSD * denominatorScalingFactor )
);
wdyt?
Go solidity wrappers are out-of-date, regenerate them via the |
// most ERC20 tokens are 18 decimals, we support tokens with up to 24 decimals | ||
if (config.decimals > 24) { | ||
revert InvalidToken(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiler will not be happy with this. our billingToken is of type IERC20 which doesnt have decimals().
Can we change from IERC20 to something that supports the decimals() function? How about this guy?
We can alias the import so it's a little cleaner...
import {IERC20Metadata as IERC20} from ...
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really well done 🎉
@@ -612,23 +635,51 @@ contract SetConfig is SetUp { | |||
); | |||
} | |||
|
|||
function testSetConfigRevertDueToInvalidDecimals() public { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome 🙌 love me some good defensive coding
|
||
uint256[] memory minRegistrationFees = new uint256[](billingTokens.length); | ||
minRegistrationFees[0] = 100000000000000000000; // 100 USD | ||
minRegistrationFees[0] = 100e18; // 100 USD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice 😎
// most ERC20 tokens are 18 decimals, priceFeed must be 8 decimals | ||
if (config.decimals != token.decimals() || config.priceFeed.decimals() != 8) { | ||
revert InvalidToken(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌 love it
No description provided.