-
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
add UpkeepCharged event #13096
add UpkeepCharged event #13096
Conversation
@@ -213,7 +213,7 @@ contract AutomationRegistry2_3 is AutomationRegistryBase2_3, OCR2Abstract, Chain | |||
emit UpkeepPerformed( | |||
report.upkeepIds[i], | |||
upkeepTransmitInfo[i].performSuccess, | |||
receipt.gasReimbursementInJuels + receipt.premiumInJuels, // TODO - this is currently the LINK amount, but may change to billing token | |||
receipt.gasReimbursementInJuels + receipt.premiumInJuels, |
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.
Should we nuke those 2 receipt related fields given that we have UpkeepCharged now? Not sure if we have backward compatibility issue, needs to confirm.
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 need to keep this field for backwards compatibility (sadly 😞)
- can we change this to be the user's billing token? ex
receipt.gasInBIllingToken + receipt.premiumInBilliingToken
??
@@ -213,7 +213,7 @@ contract AutomationRegistry2_3 is AutomationRegistryBase2_3, OCR2Abstract, Chain | |||
emit UpkeepPerformed( | |||
report.upkeepIds[i], | |||
upkeepTransmitInfo[i].performSuccess, | |||
receipt.gasReimbursementInJuels + receipt.premiumInJuels, // TODO - this is currently the LINK amount, but may change to billing token | |||
receipt.gasReimbursementInJuels + receipt.premiumInJuels, |
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 need to keep this field for backwards compatibility (sadly 😞)
- can we change this to be the user's billing token? ex
receipt.gasInBIllingToken + receipt.premiumInBilliingToken
??
receipt.linkUSD = SafeCast.toUint96(paymentParams.linkUSD); | ||
receipt.nativeUSD = SafeCast.toUint96(paymentParams.nativeUSD); | ||
receipt.billingUSD = SafeCast.toUint96(paymentParams.billingTokenParams.priceUSD); |
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.
is uint96
definitely big enough to hold these? based on the struct, we could make these uint128
with no additional storage requirements
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 uint96 should be enough:
The max number we can hold with uint96 is approximately 10^27.
For the 3 rates, we require the the Feed to be 8 decimals. So the ratio can be as big as 10^19 for Native, LINK and Billing Token.
Increasing from uint96 to uint128 seems to require one more word since we added (IERC20:160 bits, linkUSD: 128/96 bits, nativeUSD 128/96 bits and billingUSD 128/96 bits)
Let me know if the above makes sense. Not a strong opinion, happy to update~
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.
SGTM 👍
@@ -2799,6 +2799,7 @@ describe('AutomationRegistry2_3', () => { | |||
} | |||
} | |||
|
|||
// TODO |
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.
is there still something here to do?
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.
oh no, my bad. should remove this :)
receipt.linkUSD = SafeCast.toUint96(paymentParams.linkUSD); | ||
receipt.nativeUSD = SafeCast.toUint96(paymentParams.nativeUSD); | ||
receipt.billingUSD = SafeCast.toUint96(paymentParams.billingTokenParams.priceUSD); |
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.
SGTM 👍
|
No description provided.