Skip to content

Conversation

@apopiak
Copy link
Contributor

@apopiak apopiak commented Jun 14, 2022

Use the TransferFees type introduced in galacticcouncil/warehouse#42 in the runtimes and fix various type/integration issues.

Related Issue

Fixes: #412

Motivation and Context

This transfers fees to the treasury instead of burning them.
In addition it also unblocks other PRs depending on a recent version of the warehouse.

How Has This Been Tested?

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 marked this pull request as ready for review June 14, 2022 14:19
@apopiak apopiak requested a review from mrq1911 June 14, 2022 14:19
@github-actions
Copy link

github-actions bot commented Jun 14, 2022

Runtime version has not been increased.

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #493 (8b284b4) into master (60a8b41) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #493   +/-   ##
=======================================
  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 60a8b41...8b284b4. Read the comment docs.

Copy link
Member

@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.

it looks good to me. it is just an integration of work which hopefully has been done and tested.

I'll leave to @Roznovjak to approve.

Copy link
Collaborator

@Roznovjak Roznovjak 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. There is probably no documentation requiring changes. But I'm not sure about tests. Do we want to add some tests? The problem with testing is that I'm not sure how we can call a TX and charge a fee for it without using dedicated struct for handling fees (ChargeTransactionPayment, as used in the tests in the TX multi payment pallet).

@enthusiastmartin
Copy link
Member

Looks good. There is probably no documentation requiring changes. But I'm not sure about tests. Do we want to add some tests? The problem with testing is that I'm not sure how we can call a TX and charge a fee for it without using dedicated struct for handling fees (ChargeTransactionPayment, as used in the tests in the TX multi payment pallet).

i believe an integration test would be quite useful for this feature. Or update existing one to check if fees are correctly transferred to treasury.

Cargo.toml Outdated
pallet-transaction-multi-payment = { git = "https://github.com/galacticcouncil//warehouse", rev = "c73846ec093357fea1b35da5c56ecb37592d4896"}
pallet-asset-registry= { git = "https://github.com/galacticcouncil//warehouse", rev = "c73846ec093357fea1b35da5c56ecb37592d4896"}
hydradx-traits = { git = "https://github.com/galacticcouncil//warehouse", rev = "c73846ec093357fea1b35da5c56ecb37592d4896"}
pallet-price-oracle = { git = "https://github.com/galacticcouncil//warehouse", rev = "50c82ab02bb5baf99b263342dc5d4cc7e62c7e88"}
Copy link
Member

Choose a reason for hiding this comment

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

Why we dont use the very latest warehouse revision?

this one : 7052eeab309742dccfe7e307c15fc4f780713553

Copy link
Member

Choose a reason for hiding this comment

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

I have updated to the latest revision.

@enthusiastmartin enthusiastmartin merged commit 8e1ca3f into master Jun 16, 2022
@apopiak apopiak deleted the apopiak/fee-receiver branch June 20, 2022 10:39
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.

Fee's for execution goes to Treasury

4 participants