Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Asset from polkadot #1155
Asset from polkadot #1155
Changes from 2 commits
7501630
e0c3581
f070e16
780c7bd
27353f0
a543f0b
6212597
d28d9df
c23f8bb
8c8afac
0b3344c
2d17fca
047becf
3fc6cab
e53afd0
5e9820b
fddb9cf
e5d9dd8
9f7e99b
724c649
6eb8344
7c29684
edccccd
c170eea
953245b
adc73a3
28f513d
9b7811d
b00538f
3da6be4
e051f4c
79c1ba0
fdf15aa
1e6dac0
3439213
c2adf8f
b0b582d
985115f
ec33548
32f8d86
b2a666a
17f328b
6148c03
4075c11
fcd7539
dc78276
07545cf
03cf01c
e9013cd
a028d5c
3b7e178
63e8322
c887e7e
f892da4
84bb52c
64b9ef4
de3863c
25092dd
001c990
324cf53
fe4cb73
46bc45f
cf596f3
6efc7a1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe a better name for this would be something like
WrappedPolkadotAsset
or something more descriptive. This name feels extremely generic. We can also specialize this contract with thesetOwner
function and events for minting, burning, transfer, etc.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 prefer the name
ERC20
at least for the base contract.However, along the lines you suggest, we could add a child contract
WrappedToken
that inherits fromERC20
, with specialized functions for burning and anything else needed.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.
Actually, I think we should just leave the naming alone for now while we sort out the more important protocol-level issues you raised in other comments.
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 should move most of these external functions, especially the larger ones, to a common library, to save on deployment costs. Especially since this code is going to be deployed hundreds of times. It will save on bridging fees.
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.
maybe we should just be deploying simple proxies for each token rather than a full erc20 contract each time? but it's not a big deal i guess, even hundreds of times of redeploys won't cost too much
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.
Yeah, maybe we can delegate these advanced features(allowance/permit) to something like /~https://github.com/Uniswap/permit2
Currently, the ERC20.sol is only 4KB so may not be a big issue.
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.
Yeah, I was thinking about this yesterday actually. It seems ERC-6909 is the emerging standard for multi-token contracts (replacing ERC-1155). Its being used in Uniswap-v4 And so I was thinking Gateway can maintain a single ERC-6909 contract, and then instantiate thin ERC20 proxies over it.
I am ambivalent about it, which is why I didn't suggest it previously.
Positives:
Negatives:
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 make a decision here? maybe best to keep it simple and stick with existing standards?
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.
Yes, I am favor the current approach - just brought the idea up in case anyone had strong opinions.
@yrong lets go with the current approach, but with most of
ERC20.sol
factored out into a libraryERC20Lib.sol
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.
Vincent, so which functions you'd like to move to a library?
I would assume the contract size may not be a big issue here and will not make a difference for the bridging fees?
If we do want to slim the contract somehow, I'd prefer to remove all token permit/approve functions to make
ERC20.sol
minimal.For all EIP-2612, EIP-1271 features we delegate to a separate contract like permit2.
so 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.
As per this permit2 explainer, permit2 is really meant to be compatibility layer for older tokens that don't support EIP-2612. Its not meant to be a replacement for it.
And the user still has to manually approve the permit2 contract to spend the tokens. So there is an initial setup step that detracts from the UX experience.
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 don't think it will take long to introduce
ERC20Lib.sol
. The hardest part will be moving the token storage into a struct like /~https://github.com/aragon/zeppelin-solidity/blob/23540364330f6b109980933c032238dd4d1308d1/contracts/token/ERC20Lib.sol#L8After that's done, I would suggest moving
_update
into the library first, as its the largest functionThere 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.
985115f