Update fee calculation logic #50

danforbes · 2020-05-19T20:09:59Z

Closes #45

athei

I do not feel qualified to do a proper review here as I have minimal experience with javascript or even typescript. So take my comments with a grain of salt.

Someone also needs to check if all calculations are done saturating rather than overflowing. I would assume that the plain mul and add functions have overflowing semantics.

src/ApiHandler.ts

amaury1093

Code lgtm overall, added questions/nits here and there

src/ApiHandler.ts

src/main.ts

src/ApiHandler.ts

amaury1093

lgtm 👍

I have some doubts on this line:

substrate-api-sidecar/src/ApiHandler.ts

Line 143 in 7d1465e

    
           const targetedFeeAdjustment = (await api.query.transactionPayment.nextFeeMultiplier.at(parentHash)).div(perBill); // FIXME This might round the number

To convert multiplier (a Fixed128) into Perbill, we do .div(1e9). But this conversion might come with precision loss.

maciejhirsz · 2020-05-26T11:39:38Z

src/ApiHandler.ts

+		const extrinsicBaseWeight = api.consts.system.extrinsicBaseWeight;
+		// Here we assume that `weightToFee` Vec only has 1 element, which is of
+		// degree 1. FIXME Implement polynomial.
+		const coefficients = api.consts.transactionPayment.weightToFee[0];


I'm getting Internal Error: TypeError: Cannot read property '0' of undefined here. Not latest binary, but latest block.

This field is a recent addition. You need to update your node.

@athei still having the same issue on stable Kusama 0.7.33 release, fully synced.

The change is not tagged yet. You need to build master.

athei

This still has issues. Our naive javascript PerBill and Fixed128 implementations do not exactly match the rust implementation.

For that reason we will delegate the actual computation to a wasm blob that uses the real Rust types for arithmetic. sidecar just calls into that blob and supplies all data.

I take over that PR if that is ok @danforbes .

Also substrate is currently buggy and does not apply negative fee adjustments: paritytech/substrate#6145

amaury1093 · 2020-05-26T16:42:06Z

Is there a reason why we cannot do the calculations with BNs?

athei · 2020-05-26T17:51:40Z

You can do that sure. But just look at the PerBill Rust implementation. Good look replicating that so that it works in all corner cases.

Doing that in wasm is just way easier.

maciejhirsz · 2020-05-26T17:59:09Z

Riot is acting up and it seems I can't join channels. WASM is fine by me.

amaury1093 · 2020-05-26T18:09:32Z

@athei makes sense. Note that https://github.com/polkadot-js/wasm exists, the idea was to have one repo for wasm so that we don't replicate rust->wasm+js_glue_code tooling. If this is something not-sidecar-specific, it might be a good idea to put it there.

athei · 2020-05-26T18:19:40Z

Riot is acting up and it seems I can't join channels. WASM is fine by me.

I cannot accept nor decline your DM invitation. Something is seriously wrong. Try restarting :D

@athei makes sense. Note that https://github.com/polkadot-js/wasm exists, the idea was to have one repo for wasm so that we don't replicate rust->wasm+js_glue_code tooling. If this is something not-sidecar-specific, it might be a good idea to put it there.

How would I include the wasm blob into the sidecar then? Via npm.js?

amaury1093 · 2020-05-26T18:20:27Z

Via npm.js?

Yes.

Not yet functional. Some rounding differences are still present.

This also removes the contraint that only one term of the weight to fee polynomial is used.

athei · 2020-05-27T14:41:13Z

I added the code that uses a rust module to calculate the fees. It works for me and yields the correct result. I also rebased it onto the current master.

CI is currently failing because it does not build the rust package nor does it has the required toolchain installed. I will fix this soon.

athei

CI is fixed. It builds the rust project now. Let me just approve my own changes :D.

We should still wait for more tests and approvals before merging this.

athei · 2020-06-04T08:11:44Z

calc-fee/Cargo.toml

+serde_derive = { version = "1", default_features = false }
+serde = { version = "1", default_features = false }
+console_error_panic_hook = { version = "0.1", optional = true }
+sp-arithmetic = { git = "https://github.com/paritytech/substrate", default_features = false }


We should replace that by a released version as soon as that is possible. The current release on crates.io (2.0.0-rc2) has a broken Fixed128 impl that does not work with negative coefficients on unsigned Balances.

This reverts commit d098672.

danforbes requested review from maciejhirsz, athei and joepetrowski May 19, 2020 20:09

athei reviewed May 20, 2020

View reviewed changes

amaury1093 reviewed May 20, 2020

View reviewed changes

danforbes force-pushed the update-fees branch from 7055015 to 6efeb05 Compare May 20, 2020 18:56

danforbes mentioned this pull request May 20, 2020

Add WeightToFeeCoefficient type polkadot-js/api#2296

Merged

joepetrowski reviewed May 21, 2020

View reviewed changes

src/ApiHandler.ts Outdated Show resolved Hide resolved

src/ApiHandler.ts Outdated Show resolved Hide resolved

danforbes force-pushed the update-fees branch 5 times, most recently from ba3646d to 8b4a95d Compare May 21, 2020 18:42

amaury1093 reviewed May 22, 2020

View reviewed changes

src/ApiHandler.ts Outdated Show resolved Hide resolved

danforbes force-pushed the update-fees branch 2 times, most recently from ed16438 to f238657 Compare May 22, 2020 13:59

amaury1093 reviewed May 22, 2020

View reviewed changes

src/ApiHandler.ts Outdated Show resolved Hide resolved

src/ApiHandler.ts Outdated Show resolved Hide resolved

amaury1093 reviewed May 22, 2020

View reviewed changes

src/ApiHandler.ts Outdated Show resolved Hide resolved

danforbes force-pushed the update-fees branch from f238657 to 42e0f56 Compare May 22, 2020 14:26

amaury1093 approved these changes May 22, 2020

View reviewed changes

maciejhirsz reviewed May 26, 2020

View reviewed changes

athei requested changes May 26, 2020

View reviewed changes

danforbes added 2 commits May 27, 2020 16:34

Update fee calculation logic

47cea2c

Address code review by @athei and @amaurymartiny

8ce7eaf

danforbes and others added 10 commits May 27, 2020 16:34

Remove Address and LookupSource types per @amaurymartiny

2409f63

Bump Polkadot JS deps and remove WeightToFeeCoefficient type

a7b8206

Use variables consistently

28659ab

Clean up diff

4dda869

Update lockfile, formatting changes

56f697f

Update fee calculation logic

eb3211a

Rename variables, better comments (#54)

006e984

Upgraded to latest polkadot API release

5b3b643

Fixes for weight calculation

017fc30

Not yet functional. Some rounding differences are still present.

Calculate extrinsic fees in rust

1d3f87f

This also removes the contraint that only one term of the weight to fee polynomial is used.

athei force-pushed the update-fees branch from c2e50e2 to 1d3f87f Compare May 27, 2020 14:38

Fix CI

52d91dc

athei approved these changes May 27, 2020

View reviewed changes

athei mentioned this pull request May 27, 2020

Make post dispatch fee consistent with a direct calculation paritytech/substrate#6165

Merged

This was referenced Jun 3, 2020

Update README.md to have wasm-pack install instructions #65

Closed

Add wasm-pack install instructions to README.md #66

Merged

athei reviewed Jun 4, 2020

View reviewed changes

athei added 9 commits June 6, 2020 15:13

Merge branch 'master' into update-fees

f723c34

Reproduce fixed128 bug that was deployed to polkadot v0.

7139830

Support kusama without queryInfo

8aa6f1f

Do not fail the whole request when runtime is not supported

1ab5267

Do not send per block parameters for every extrinsic to wasm

3c21419

Install wasm-pack through node

d098672

Merge branch 'master' into update-fees

39c2796

Remove no longer needed bn.js dependency

8c2230e

Revert "Install wasm-pack through node"

253989e

This reverts commit d098672.

joepetrowski approved these changes Jun 8, 2020

View reviewed changes

joepetrowski merged commit f5b8427 into master Jun 8, 2020

joepetrowski deleted the update-fees branch June 8, 2020 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update fee calculation logic #50

Update fee calculation logic #50

danforbes commented May 19, 2020

athei left a comment

amaury1093 left a comment

amaury1093 left a comment

maciejhirsz May 26, 2020

athei May 26, 2020

maciejhirsz May 26, 2020

athei May 26, 2020

athei left a comment •

edited

Loading

amaury1093 commented May 26, 2020

athei commented May 26, 2020 •

edited

Loading

maciejhirsz commented May 26, 2020

amaury1093 commented May 26, 2020 •

edited

Loading

athei commented May 26, 2020

amaury1093 commented May 26, 2020

athei commented May 27, 2020

athei left a comment •

edited

Loading

athei Jun 4, 2020

Update fee calculation logic #50

Update fee calculation logic #50

Conversation

danforbes commented May 19, 2020

athei left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

amaury1093 left a comment

Choose a reason for hiding this comment

maciejhirsz May 26, 2020

Choose a reason for hiding this comment

athei May 26, 2020

Choose a reason for hiding this comment

maciejhirsz May 26, 2020

Choose a reason for hiding this comment

athei May 26, 2020

Choose a reason for hiding this comment

athei left a comment • edited Loading

Choose a reason for hiding this comment

amaury1093 commented May 26, 2020

athei commented May 26, 2020 • edited Loading

maciejhirsz commented May 26, 2020

amaury1093 commented May 26, 2020 • edited Loading

athei commented May 26, 2020

amaury1093 commented May 26, 2020

athei commented May 27, 2020

athei left a comment • edited Loading

Choose a reason for hiding this comment

athei Jun 4, 2020

Choose a reason for hiding this comment

athei left a comment •

edited

Loading

athei commented May 26, 2020 •

edited

Loading

amaury1093 commented May 26, 2020 •

edited

Loading

athei left a comment •

edited

Loading