-
Notifications
You must be signed in to change notification settings - Fork 108
prototype: remote superchain erc20 #373
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
base: graphite-base/373
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Deploying supersim with
|
| Latest commit: |
1ac71e0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4876563d.supersim.pages.dev |
| Branch Preview URL: | https://phantom-superchain-erc20.supersim.pages.dev |
101d07a to
a408858
Compare
| import {IL2ToL2CrossDomainMessenger} from "@contracts-bedrock-interfaces/L2/IL2ToL2CrossDomainMessenger.sol"; | ||
| import {Predeploys} from "@contracts-bedrock/libraries/Predeploys.sol"; | ||
|
|
||
| contract PhantomSuperchainERC20 is ERC20 { |
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 worry that this ERC20 is too feature rich and will cause foot guns. For example things that won't work well:
- Approve
- TotalSupply
because they will all return values which seem correct but actually are broken. If you give someone an API, they will use it even if you don't intend for them to use it.
I think a way to adjust this is to actually revert if anything is called that we can't support 1:1 with an ERC20. That way, when integrating this into an existing application we can also get noisy errors if someone integrates the phantom and then their tests fail because they use approve
Oh another note -- the API we support is actually a bit nuanced because we don't actually fully support transferFrom or balanceOf. We ONLY support transferFrom(addr) or balanceOf(addr) if the addr==the contract which is using the token. So we should enforce that behavior as well
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.
yea 100%, inheriting ERC20 here was just to bootstrap this implementation and not concern myself with having erc20 stuff implemented.
transferFrom: agreed we should not implement this. However this API could by default be thecrossDomainDepositFromin the context of the phantom token to remotely transfer the funds into a recipient address. Kinda fits the name nicely. Only the messenger would be authorized asmsg.sender.
++1 on approvals and such
8a726f1 to
a2317fa
Compare
0584150 to
0ec7efe
Compare
e68584a to
1ac71e0
Compare

Uh oh!
There was an error while loading. Please reload this page.