Erc20 XCM bridge #2090
Merged
Erc20 XCM bridge #2090
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
librelois
added
B7-runtimenoteworthy
4meta5
reviewed
Feb 10, 2023
Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
Conflicts: Cargo.lock runtime/common/Cargo.toml runtime/moonbase/Cargo.toml runtime/moonbase/src/lib.rs
tgmichel
reviewed
Feb 16, 2023
girazoki
reviewed
Feb 16, 2023
| impl pallet_xcm::Config for Runtime { | ||
| type RuntimeEvent = RuntimeEvent; | ||
| type SendXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>; | ||
| type XcmRouter = XcmRouter; | ||
| type ExecuteXcmOrigin = EnsureXcmOrigin<RuntimeOrigin, LocalOriginToLocation>; | ||
| type XcmExecuteFilter = Everything; | ||
| type XcmExecuteFilter = XcmExecuteFilter; |
There was a problem hiding this comment.
Why are we changing this?
There was a problem hiding this comment.
I added more documentation about this filter, I took the opportunity to improve it and move it to the erc20 xcm pallet.
girazoki
reviewed
Feb 16, 2023
girazoki
reviewed
Feb 16, 2023
girazoki
reviewed
Feb 16, 2023
girazoki
reviewed
Feb 16, 2023
girazoki
reviewed
Feb 16, 2023
| /// some XCM instructions if they could manipulate erc20 assets. | ||
| /// If your runtime allows arbitrary xcm messages to be executed locally you should use this | ||
| // wrapper. | ||
| pub struct XcmExecuteFilterWrapper<Runtime, FallbackFilter>( |
There was a problem hiding this comment.
I dont mind leaving this filter, but I dont think it adds much value TBH. InitiateReserveWithdraw will instruct our sovereign account in another chain to withdraw the ERC20, but our sovereign account in another chain will likely not contain these (unless someone else has put it there on purpose).
There was a problem hiding this comment.
In other words the message will fail. This should be no security risk, if something we are telling users not to try this because their message will fail. And I am not a big fan of the latter...
girazoki
approved these changes
Feb 16, 2023
There was a problem hiding this comment.
Good job @librelois, I just have a comment on executeFilter that I dont think it adds much value, but other than that looks good!
notlesh
approved these changes
Feb 17, 2023
notlesh
added
D1-audited👍
imstar15
pushed a commit
to OAK-Foundation/moonbeam
that referenced
this pull request
May 16, 2023
* prepare evm runner for erc20 bridge * upgrade frontier * catch ethereum execution informations * add pallet erc20-xcm-bridge * add bridge supply counter * impl internal_transfer_asset for Erc20XcmBridge * add xcm holding extension "erc20s origins" * inject precompile handle in substrate calls triggered by precompiles * rename rust module evm_runner * fix: xtokens should be able to perform evm subcall * fix: adress without know prefix should resolve into erc20 currency * add ts test "transfer ERC20 token throught xcm with xtokens precompile" * Reserve pallet id 109 for future integration in moonriver/moonbeam * fmt * environmental/std * editorconfig * rm unreachable! * toml-sort * clean some unused code * forbid-evm-reentrancy * impl an AssetTrap that filter out erc20 assets * Update runtime/moonbase/src/xcm_config.rs Co-authored-by: Amar Singh <asinghchrony@protonmail.com> * ref: replace xcm holding ext by asset multilocation ext * Merge branch 'master' into elois-erc20-xcm-bridge * update frontier * fix compilation warning * erc20_transfer: opt call data allocation * Revert "replace xcm holding ext by asset multilocation ext" * remove duplicated code * add rust unit tests for Erc20Matcher * configurable erc20 transfer gas limit * get evm config from pallet evm config * remove reserved pallet id for moonriver/moonbeam * perform erc20 transfer in a db transaction in all cases * charge weigh for xcm erc20 transfers according to configured gas limit * filter some xcm instructions for arbitrary local execution * apply suggestion * 80000 gas limit is enough for almost all erc20 implementations * more doc * optimize is_erc20_asset and use it on trap * each withdrawal of erc20 tokens must be deposited at once * improve XcmExecuteFilter impl and move it in pallet erc20 xcm * call inner filter --------- Co-authored-by: Alan Sapede <alan@purestake.com> Co-authored-by: Amar Singh <asinghchrony@protonmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What does it do?
Add ability to transfer any ERC20 token through xcm directly.
The implementation is mainly about adding a new
TransactAssetimplementation that allow the xcm executor to handle erc20 assets.Breaking changes
currencyIdwill always resolve to erc20 asset as a default instead of throwing revert error"Cannot convert into currency id".TransferAsset,TransferAssetReserveAssetandWithdrawAssetwill cost2_000_000_000weights per erc20 asset instead of200_000if they not contains any erc20 asset.InitiateReserveWithdrawandInitiateTeleportare now forbidden if they can contains erc20 assets.Security considerations for erc20 contracts
If the implementation of the
transferfunction returns true when the transfer has not really been done, it allows to duplicate tokens. Indeed, in the case of sending tokens to another chain, they must be transferred to the sovereign account of the destination chain, if the contract returns true, the xcm protocol will consider that the tokens have been transferred to the sovereign account of the destination chain and will send the tokens on the destination chain.We consider that it is the responsibility of each erc20 contract to handle error cases correctly, and to return true only if the transfer really went well.
What important points reviewers should know?
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
What value does it bring to the blockchain users?
TODO