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

393 use fee per second from asset metadata in xcm config #432

Merged
merged 84 commits into from
Apr 8, 2024
Merged

393 use fee per second from asset metadata in xcm config #432

merged 84 commits into from
Apr 8, 2024

Conversation

ebma
Copy link
Member

@ebma ebma commented Mar 14, 2024 •
edited by gianfra-t
Loading

This PR adds changes that were reverted before. The original PR is #410.

We only want to merge this after the dependency upgrade to v0.9.42 is merged and released.

TODO

  • We still need to change the conversion function for MultiLocation <> CurrencyId. Currently, we are still using the hard-coded logic that manually maps these two but we also want to use the asset registry for this so no further runtime upgrades are needed for adding assets. Thus, we should change the conversion function similar to what is done here.

Closes #393.

@bogdanS98
WIP
b1ad3f1
@bogdanS98
Fmt
dd83e1b
@bogdanS98
Add comment
3ff7277
@bogdanS98
Still wip
dcc6a4b
@bogdanS98
Fmt
d998215
@bogdanS98
Add doc comments
0afee4a
@bogdanS98
Address change requests
3e7b9bb
@bogdanS98
Fmt
edc4d94
@bogdanS98
Add comment
1359c94
@bogdanS98
Add fee for basic asset
dc68751
@bogdanS98
Fmt
069d17e
@bogdanS98
Return NoPrice error when price fetching fails
67f3513
@bogdanS98
Add signed extension
beb234b
@bogdanS98
Add first iteration of mock
2479198
@bogdanS98
Allow only XCM assets for buyout
449296a
@bogdanS98
Update mock to only allow specific XCM assets
69a81c7
@bogdanS98
Add tests and benchmarks - still wip
b8cda83
@bogdanS98
Fmt
95035a4
@bogdanS98
Handle currency id in benchmarks
eebf511
@bogdanS98
Update comments
a75ad1a
@bogdanS98
Remove benchmark comment
560268b
@bogdanS98
Fix benchmarks and tests, add pallet to Foucoco, switch from UnixTime…
be4f99a 
… to blocks, generate weights
@bogdanS98
Merge remote-tracking branch 'origin' into 395-implement-pallet-that-…
19932f9 
…extends-our-current-treasury-pallet-with-buyout-feature
@bogdanS98
Fmt
4aaee33
@bogdanS98
Refactor dot currency id in tests
f2d1eda
@bogdanS98
Small refactor for test attempt_buyout_after_buyout_limit_exceeded_fails
8bae6e8
@bogdanS98
Update comments
11d9609
@bogdanS98
Small refactor
d224292
@bogdanS98
Add comment in Foucoco runtime
e07318c
@bogdanS98
Add comment in Foucoco runtime
fa68d4d
@gianfra-t gianfra-t marked this pull request as ready for review March 21, 2024 15:15
@gianfra-t gianfra-t requested a review from a team March 21, 2024 15:15
ebma
ebma commented Mar 22, 2024
View reviewed changes
runtime/foucoco/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/foucoco/src/xcm_config.rs Outdated Show resolved Hide resolved
@ebma ebma mentioned this pull request Mar 27, 2024
Merged
ebma
ebma commented Apr 2, 2024
View reviewed changes
runtime/amplitude/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/foucoco/src/xcm_config.rs Outdated Show resolved Hide resolved
TorstenStueber
TorstenStueber requested changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

Overall a good change but I think we should simplify some of the types – I made some proposals in my comments.

node/src/chain_spec.rs Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
runtime/common/src/lib.rs Outdated Show resolved Hide resolved
runtime/common/src/lib.rs Outdated Show resolved Hide resolved
runtime/amplitude/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/foucoco/src/xcm_config.rs Show resolved Hide resolved
@gianfra-t
Copy link
Contributor

Last commit updated and simplified the implementation now. I will leave a brief description since the conversation is now too long.

We now don't modify at all the location before trying to convert nor we convert while getting the fee per second.
This is all possible because now we use the local PoV for our assets in the asset registry (example in the mock). xTokens::tansfer_multiasset now works with the local PoV (Thanks @TorstenStueber for figuring that out)

@TorstenStueber
Copy link
Contributor

The Foucoco SelfLocation is still (parents: 1, Parachain(...)), can you change that please?

TorstenStueber
TorstenStueber approved these changes Apr 5, 2024
View reviewed changes
Copy link
Contributor

@TorstenStueber TorstenStueber left a comment

Choose a reason for hiding this comment

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

This turned out really nice now. Great job!

@ebma ebma changed the title [On Hold] 393 use fee per second from asset metadata in xcm config 393 use fee per second from asset metadata in xcm config Apr 8, 2024
@ebma
Copy link
Member Author

ebma commented Apr 8, 2024

Let's merge and roll it out on Foucoco today?

@ebma ebma merged commit b39b8ef into main Apr 8, 2024
2 checks passed
@ebma ebma deleted the 393-use-fee_per_second-from-asset-metadata-in-xcm-config branch April 8, 2024 12:35
@gianfra-t gianfra-t mentioned this pull request Apr 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianfra-t gianfra-t gianfra-t left review comments

@TorstenStueber TorstenStueber TorstenStueber approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use fee_per_second from asset metadata in XCM config
4 participants
@ebma @gianfra-t @TorstenStueber @bogdanS98