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.

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

feat!: Set Transaction Fee Asset on New Account #476

Merged
merged 22 commits into from
Jun 16, 2022

Conversation

apopiak
Copy link
Collaborator

@apopiak apopiak commented Jun 7, 2022

Use the types introduced in galacticcouncil/warehouse#54 to set transaction fee asset on new account (and clean up when fee asset balance reaches zero or system account is reaped).

Description

As described in #454 we want to automatically set a user's fee asset when it makes sense to do so.

We do this by implementing a new callback introduced to orml_tokens that gets triggered whenever a new account is created for a (asset_id, account_id) combination. If the account id does not have a fee currency set and the asset id of the new account is an accepted fee currency, we set the user's fee currency to this new asset.

Notes

The weights should be rerun because this changes the costs of creating and deleting accounts.

Related Issue

Fixes: #454

How Has This Been Tested?

  • local integration test
  • xcm transfer integration test

Checklist:

  • I have updated the documentation if necessary.
  • I have added tests to cover my changes, regression test if fixing an issue.
  • This is a breaking change.

@apopiak apopiak requested review from mrq1911 and jak-pan June 7, 2022 09:58
@github-actions
Copy link

github-actions bot commented Jun 7, 2022

Crate versions that have been updated:

  • runtime-integration-tests: v0.3.0 -> v0.3.1
  • pallet-duster: v2.3.0 -> v2.3.1
  • pallet-exchange: v5.3.2 -> v5.3.3
  • pallet-exchange-benchmarking: v3.3.12 -> v3.3.13
  • pallet-lbp: v4.4.2 -> v4.4.3
  • pallet-liquidity-mining: v1.2.3 -> v1.2.4
  • pallet-liquidity-mining-benchmarking: v1.0.3 -> v1.0.4
  • pallet-xyk: v3.6.2 -> v3.6.3
  • basilisk-runtime: v52.0.0 -> v53.0.0
  • common-runtime: v1.9.2 -> v1.9.3
  • testing-basilisk-runtime: v52.0.0 -> v53.0.0

Runtime version has been increased.

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #476 (e18d351) into master (037fab9) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #476   +/-   ##
=======================================
  Coverage   83.44%   83.44%           
=======================================
  Files          24       24           
  Lines        2935     2935           
=======================================
  Hits         2449     2449           
  Misses        486      486           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 037fab9...e18d351. Read the comment docs.

@apopiak apopiak marked this pull request as ready for review June 10, 2022 15:23
@apopiak apopiak requested a review from enthusiastmartin June 10, 2022 15:45
Copy link
Contributor

@enthusiastmartin enthusiastmartin left a comment

Choose a reason for hiding this comment

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

This looks ok.

I wonder where the update of event structs should be as part of standalone PR so it can be easily tracked ( because UI needs to be aware ).

Cargo.toml Outdated Show resolved Hide resolved
pallets/xyk/src/tests.rs Outdated Show resolved Hide resolved
(66_923_000 as Weight)
.saturating_add(T::DbWeight::get().reads(4 as Weight))
.saturating_add(T::DbWeight::get().writes(3 as Weight))
(66_005_000 as Weight)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these extra reads and writes part of our change or is this unrelated orml thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

i believe it is the extra read and write of "set fee asset" handler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Jep, that's why I asked to run the weights, cause I wanted us to see the impact.

@mrq1911
Copy link
Member

mrq1911 commented Jun 14, 2022

Task linked: CU-2kpe21t Set Fee Asset Id on New Account

@apopiak
Copy link
Collaborator Author

apopiak commented Jun 14, 2022

requires #493 (and includes its changes)

@enthusiastmartin enthusiastmartin merged commit e4491ab into master Jun 16, 2022
@apopiak apopiak deleted the apopiak/set-tx-fee-asset-on-new-account branch June 20, 2022 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change fee payment token on account create, transfer or xcm transfer
4 participants