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

Add minimal tests to show that new runtime API endpoints exist #2254

Merged
merged 17 commits into from
May 18, 2023

Conversation

notlesh
Copy link
Contributor

@notlesh notlesh commented Apr 20, 2023

What does it do?

This is the first pass at using the new transaction-payment runtime API added in Substrate.

It will at least add test coverage demonstrating that the API works and how it can be used to calculate the entire fee for a Substrate-based transaction.

As a reminder, this was added so that we can run exhaustive tests around fee calculation, which we were doing until it broke in paritytech/substrate#11415 when it became impossible to know all fee parameters from a client alone.

@notlesh notlesh added B0-silent Changes should not be mentioned in any release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes labels Apr 20, 2023
describeDevMoonbeam("TransactionPayment Runtime Queries", (context) => {
it("should be able to query length fee", async function () {
const adjusted_length_fee = await context.polkadotApi.call.transactionPaymentApi.queryLengthToFee(1n);
expect((adjusted_length_fee as any).toBigInt()).to.eq(1_000_000_001n);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What can I do to avoid needing as any here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Updates the typescript api to match

cd typescript-api
npm ci
cd ../tests
npm run setup-typescript-api

Copy link
Contributor

Choose a reason for hiding this comment

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

@notlesh you dont need to build any new types or api-augment pkgs, that's been done already.

All you need todo is update the api-augment package to 2301 which contains the updated interfaces (and removes the need for any locally for me at least)

in: moonbeam/tests

npm i @moonbeam-network/api-augment@latest 

let apiAt = await context.polkadotApi.at(previousBlockHash); // TODO: i think this is right because multiplier is updated in on_finalize
let lengthFee = (await apiAt.call.transactionPaymentApi.queryLengthToFee(dispatchInfo.encodedLength) as any).toBigInt();

let unadjustedWeightFee = (await apiAt.call.transactionPaymentApi.queryWeightToFee(dispatchInfo.weight) as any).toBigInt();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something seems very broken about dispatchInfo.weight. In this particular case (running test "should be able to calculate entire fee"), it's reporting a refTime of 423314000 which is definitely wrong.

The real value should be 173314000, which is correct in fee.weight. Why is dispatchInfo so wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Furthermore, why is it used [presumably] correctly (above in the same fn) for Ethereum txns?

@notlesh notlesh requested review from timbrinded and crystalin May 8, 2023 19:11
let feePortions = calculateFeePortions(fee.partialFee.toBigInt());
txFees = fee.partialFee.toBigInt();
txBurnt += feePortions.burnt;
console.log(`evaluating substrate fee: ${fee}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be "debug" instead of console.log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, sorry. Removed in 316c698

describeDevMoonbeam("TransactionPayment Runtime Queries", (context) => {
it("should be able to query length fee", async function () {
const adjusted_length_fee = await context.polkadotApi.call.transactionPaymentApi.queryLengthToFee(1n);
expect((adjusted_length_fee as any).toBigInt()).to.eq(1_000_000_001n);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Updates the typescript api to match

cd typescript-api
npm ci
cd ../tests
npm run setup-typescript-api

refTime: 1,
proofSize: 1,
});
expect((adjusted_weight_fee as any).toBigInt()).to.eq(50_000n);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace this assertion value here is WEIGHT_FEE defined in constants.ts if you feel this is appropriate.

If not, feel free to define some constants in there with appropriate names which you want to assert against (which can then be reusable in other tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@github-actions
Copy link
Contributor

github-actions bot commented May 17, 2023

Coverage generated "Thu May 18 21:44:53 UTC 2023":
https://s3.amazonaws.com/moonbeam-coverage/pulls/2254/html/index.html

Master coverage: 72.40%
Pull coverage: 72.41%

@notlesh notlesh requested review from crystalin and timbrinded May 18, 2023 19:58
Copy link
Contributor

@timbrinded timbrinded left a comment

Choose a reason for hiding this comment

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

LGTM, but the doing the previous comment would be the 🍒 on top

@notlesh notlesh merged commit cb01851 into master May 18, 2023
@notlesh notlesh deleted the notlesh-tp-runtime-api branch May 18, 2023 21:49
timbrinded pushed a commit that referenced this pull request Jun 2, 2023
* Add minimal tests to show that new runtime API endpoints exist

* prettier

* WIP: trying to reassemble substrate fees

* Basic fee calc works

* prettier

* Remove dead code

* Prefer const over let 🙈

* Validate substrate tips properly (burned)

* Remove console.log()

* Use well-defined constant

* Fix merge conflict 👀

* Remove as any

* Reflect substrate tips being handled now 🎉

* prettier

* Explain the length fee calculation

* Use newly defined constant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B0-silent Changes should not be mentioned in any release notes D3-trivial PR contains trivial changes in a runtime directory that do not require an audit not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants