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

fix: support calc fee for Karura and Acala #754

Merged
merged 2 commits into from
Nov 10, 2021

Conversation

xlc
Copy link
Contributor

@xlc xlc commented Nov 9, 2021

I cannot test locally. I guess it is because we need to publish the calc package first before the wasm can be used.

export const karuraDefinitions: MetadataConsts[] = [
{
runtimeVersions: [
1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, 1008, 1009, 1010, 1011,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need to list all the runtime versions? Do we need to PR here if we did a runtime upgrade?

Copy link
Member

Choose a reason for hiding this comment

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

Yea and yes. If it doesnt have your new runtime version immediately thats alright, we just end up searching the decorated metadata for the weights for the missing runtime then cache it for the server instance.

@@ -63,6 +63,9 @@ impl Multiplier {

("calamari", _v) => V2(new_u128(inner)),

("karura", _v) => V2(new_u128(inner)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be a way to configure this without modify rust code. at least for common cases.

Copy link
Member

Choose a reason for hiding this comment

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

Yea we have had discussion about this, especially since parachains are going to want more and more support for fee calculations as time moves on. We thought about a plugin, and some different options. Still something to consider but I agree.

@TarikGul
Copy link
Member

TarikGul commented Nov 9, 2021

I will test it for you as well. But if you want to test it locally it's pretty simple. Just link the calc/pkg in the package.json

So it should look like this for the @substrate/calc dep:

"@substrate/calc": "file:./calc/pkg"

Then yarn, and it will have your changes reflected locally. Just make sure not to push it up :)

@xlc
Copy link
Contributor Author

xlc commented Nov 9, 2021

I will test it for you as well. But if you want to test it locally it's pretty simple. Just link the calc/pkg in the package.json

So it should look like this for the @substrate/calc dep:

"@substrate/calc": "file:./calc/pkg"

Then yarn, and it will have your changes reflected locally. Just make sure not to push it up :)

Thanks. I guess this needs to be part of README?

@xlc
Copy link
Contributor Author

xlc commented Nov 10, 2021

Trie a few block of Karura and the number is matching values from Subscan.

@TarikGul TarikGul changed the title support calc fee for Karura and Acala fix: support calc fee for Karura and Acala Nov 10, 2021
@TarikGul
Copy link
Member

TarikGul commented Nov 10, 2021

Sweet, ran it locally and it checks out.

I guess this needs to be part of README?

Good point. I'll write up some proper guide for it.

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

clean, LGTM

@TarikGul
Copy link
Member

As a note, this will be reflected in the next release of sidecar. Merging.

@TarikGul TarikGul merged commit 42ae857 into paritytech:master Nov 10, 2021
@xlc xlc deleted the karura-acala branch November 10, 2021 20:21
This pull request was closed.
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.

4 participants