Skip to content

Conversation

@athei
Copy link
Member

@athei athei commented Jun 18, 2020

This PR adds support for the upcoming polkadot, kusama and westend runtimes. Westend starting with runtime 10 is now also support but untested.

In addition to these changes I also added the following modifications:

  • By building the calc-fee rust project with FEE_DEBUG=1 in the environment you get some nice debugging output
  • I moved the runtime switching logic to the calc-fee project.

@athei athei requested review from danforbes and emostov June 18, 2020 10:54
Copy link
Contributor

@emostov emostov left a comment

Choose a reason for hiding this comment

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

As far as I can understand it LGTM. Just had a comment about stuff to think about in the future in terms of generalized support of substrate based chains.


_ => {
info!("Unsupported runtime: {}#{}", spec_name, spec_version);
return None;
Copy link
Contributor

Choose a reason for hiding this comment

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

This warning is a good idea, and I think it works well for now. It does make me think though that in the future we should maybe think about how to make all of sidecar easily configurable for a wide array of substrate based chains. Polkadot-js already easily supports adding types, and I am wondering if we could do something of similar effect with fee stuff in this crate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of bailing we could just use the current Multiplier as long as the chain supplied a valid Polynomial. That would instantly support all current chains that use the ChargeTransactionPayment (should be most of them).

So maybe we should rather explicitly ban unsupported versions of our runtimes than disallowing anything we do not know.

Copy link
Contributor

@danforbes danforbes left a comment

Choose a reason for hiding this comment

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

Looks awesome 🚀 Noting that as we move more logic into the Rust code we will want to create Rust unit tests.

@danforbes danforbes merged commit 2d63ca7 into master Jun 18, 2020
@danforbes danforbes deleted the at-fee-debug branch June 18, 2020 16:44
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