Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optional ERC20-XCM gas limit override and tests #2422

Merged
merged 16 commits into from
Aug 28, 2023
Merged

Optional ERC20-XCM gas limit override and tests #2422

merged 16 commits into from
Aug 28, 2023

Conversation

fgamundi
Copy link
Contributor

@fgamundi fgamundi commented Aug 7, 2023 •
edited
Loading

Accept an additional optional Junction for incoming ERC20-XCM transfers to override the default gas limit Erc20XcmBridgeTransferGasLimit in the asset multilocation.

@fgamundi fgamundi added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. breaking Needs to be mentioned in breaking changes labels Aug 7, 2023
@fgamundi fgamundi added the A0-pleasereview Pull request needs code review. label Aug 7, 2023
@fgamundi fgamundi marked this pull request as ready for review August 7, 2023 14:49
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023 •
edited
Loading

Coverage generated "Fri Aug 25 07:55:46 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2422/html/index.html

Master coverage: 87.39%
Pull coverage:

@fgamundi fgamundi requested a review from librelois August 7, 2023 15:50
notlesh
notlesh reviewed Aug 7, 2023
View reviewed changes
Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, I left a couple comments.

Another more general one: GeneralIndex seems very general-purpose for this. Maybe we should create a special AssetId to represent gas instead?

librelois
librelois requested changes Aug 8, 2023
View reviewed changes
Copy link
Collaborator

@librelois librelois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is incomplete, you should also modify the XCM Weigher to apply the gas limit to the message weight and then the XCM fees.

librelois
librelois reviewed Aug 17, 2023
View reviewed changes
pallets/erc20-xcm-bridge/src/erc20_matcher.rs Outdated
@@ -67,6 +67,28 @@ impl<Erc20MultilocationPrefix: Get<MultiLocation>> Erc20Matcher<Erc20Multilocati
_ => Err(()),
}
}
pub(crate) fn matches_gas_limit(multiasset: &MultiAsset) -> Option<u64> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is useless and increase the call stack for nothing, please remove it and move it's body inside weight_of_erc20_transfer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could use if let to write this concisely instead of having a helper fn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notlesh tried to follow your suggestion, let me know what you think about the new implementation here

@librelois
Copy link
Collaborator

@fgamundi next time, please notify the reviewers again via github, otherwise I won't come back until I've reviewed all the open PRs (which is not often).

@fgamundi
Copy link
Contributor Author

Hey @librelois. Thanks for the review! I didn't request rereview because I still wanted to make more changes (partially addressing what you commented), but didn't get time before my time off

@fgamundi fgamundi requested review from notlesh and librelois August 25, 2023 00:26
@librelois librelois mentioned this pull request Aug 25, 2023
Open
28 tasks
@fgamundi fgamundi merged commit 3ebf181 into master Aug 28, 2023
@fgamundi fgamundi deleted the fg-erc20xcm-manual-gas-limit branch August 28, 2023 11:12
@fgamundi fgamundi added not-breaking Does not need to be mentioned in breaking changes and removed breaking Needs to be mentioned in breaking changes labels Aug 30, 2023
@Agusrodri Agusrodri mentioned this pull request Sep 18, 2023
Merged
@fgamundi fgamundi mentioned this pull request Sep 20, 2023
Merged
@notlesh notlesh added D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@librelois librelois librelois approved these changes

@notlesh notlesh Awaiting requested review from notlesh

Assignees
No one assigned
Labels
A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-audited👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fgamundi @librelois @notlesh