-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 EIP-5994: Token Balance Update Hooks (Pods) #6000
Conversation
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s): (fail) eip-5994.md
|
Co-authored-by: Pandapip1 <[email protected]>
EIPS/eip-5994.md
Outdated
```solidity | ||
interface IPod { | ||
function updateBalances(address from, address to, uint256 amount) external; | ||
} | ||
|
||
interface IERC20Pods is IERC20 { | ||
function hasPod(address account, address pod) external view returns(bool); | ||
function podsCount(address account) external view returns(uint256); | ||
function podAt(address account, uint256 index) external view returns(address); | ||
function pods(address account) external view returns(address[] memory); | ||
function podBalanceOf(address pod, address account) external view returns(uint256); | ||
|
||
function addPod(address pod) external; | ||
function removePod(address pod) external; | ||
function removeAllPods() external; | ||
} | ||
|
||
interface IERC721Pods is IERC721 { | ||
function hasPod(address account, address pod) external view returns(bool); | ||
function podsCount(address account) external view returns(uint256); | ||
function podAt(address account, uint256 index) external view returns(address); | ||
function pods(address account) external view returns(address[] memory); | ||
function podBalanceOf(address pod, address account) external view returns(uint256); | ||
|
||
function addPod(address pod) external; | ||
function removePod(address pod) external; | ||
function removeAllPods() external; | ||
} |
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 should be ideally be split into three sections, each with NatSpec. In addition, I think that pods, in order to be versatile enough for the use cases mentioned in the rationale, should:
- Have a hook for
addPod
that allows the pod the chance to revert and update tracking variables - Have a hook for
removePod
that allows the pod the chance to revert and update tracking variables - Have separate hooks and hook names for EIP-20, EIP-721, and EIP-1155. EIP-20 and EIP-1155 pods should take an amount, EIP-721 pods shouldn't. And EIP-721 and EIP-1155 should take a tokenId, and EIP-20 shouldn't.
- For EIP-721 and EIP-1155, pods should be able to be added per token ID or for the token as a whole.
- (This is a big one for me) EIP-165 should be supported.
Also, where is the EIP-1155 interface?
|
||
## Security Considerations | ||
|
||
Pods implementations will never block balance update, but can eat upto 200,000 gas. Number of added Pods per account is limited to some immutable value (reasonable amount taking into account max transaction gas limit). |
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.
- ERCs MUST NOT depend on gas costs. They change from one hard fork to the next. EIP-165 is the one exception to this as it was finalized before that rule was put in place. It's well-recognized that the 30,000 gas limit is not a "real" rule, however.
- This isn't in the spec!
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 see no way to avoid this constant. This number should be specified. Moreover we have restriction of number of pods added to one account. N*200k should be less than max transaction gas limit. So I expect devs to use 200k constant and 10-20 pods limit per account.
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.
Core concept is permissionless pods and risk-free participation. So Pods cant block transfers/mint/burns of designated assets.
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 okay to have the gas limit as a should. But it is not okay to have it as a must.
EIPS/eip-5994.md
Outdated
|
||
### Pod specification | ||
|
||
- Pod MUST implement interface `IPod` and fit execution of `updateBalances` method in 200,000 gas and never revert. |
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.
- Pod MUST implement interface `IPod` and fit execution of `updateBalances` method in 200,000 gas and never revert. | |
- Pod MUST implement interface `IPod` and fit execution of `updateBalances` method within 200,000 gas and never revert. |
For in
, do you mean within
?
Could you provide rationale about how the number 200,000
was picked??
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.
@xinbenlv number is relatively big for any possible logic, but small enough to have limitation 5-10 pods per account to fit into more sophisticated transaction as part of token transfer. We expect in practice users will have up to 2-3 pods with 30-50k gas consumption max for each, in average nearly 10-20k each.
|
||
## Security Considerations | ||
|
||
Pods implementations will never block balance update, but can eat upto 200,000 gas. Number of added Pods per account is limited to some immutable value (reasonable amount taking into account max transaction gas limit). |
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 am a little worry about the introduction of Pods and its hooks will massively increase the chance of renetry attack, could you provide your considerations about this?
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
--- | ||
eip: 5994 | ||
title: Token Balance Update Hooks (Pods) | ||
description: Extends EIP-20, EIP-721, EIP-1155 token standards to enable opt-in balance tracking by external contracts (Pods) |
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.
description: Extends EIP-20, EIP-721, EIP-1155 token standards to enable opt-in balance tracking by external contracts (Pods) | |
description: Extends EIP-20, EIP-721, and EIP-1155 to enable opt-in balance tracking by external contracts (Pods) |
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
Co-authored-by: Pandapip1 <[email protected]>
There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review. |
This pull request was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment. |
This EIP introduces Token Pods, enabling external smart contract based Pods to track token balances of those users who opted-in to these Pods. Pods can implement different logic, for example permissionless and risk-free farms.
Discussion:
Example implementation:
Examples of Pods: