-
Notifications
You must be signed in to change notification settings - Fork 157
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: Account for polkadot-js changes; Harden createCalcFee
#376
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
src/services/blocks/BlocksService.ts
Outdated
@@ -390,11 +394,17 @@ export class BlocksService extends AbstractService { | |||
version.specVersion.toNumber(), | |||
]; | |||
|
|||
// The compiler doesn't know this is an AbastractInt so we do a runtime check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dq: why doesn't it know that and why can't we add type annotations on line 343?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvdplm Honestly I was on the fence about it. It does not have the TS bindings defined within the API (see here: https://github.com/polkadot-js/api/blob/master/packages/api/src/augment/consts.ts). Not exactly sure why that is but just guessing the script that generates those bindings was run against a runtime that does not use the extrinsicBaseWeight
const. I think I will actually just cast it though. No matter the type it certainly will always be a descendant of AbstractInt
since it has to be some type of integer.
Co-authored-by: David <dvdplm@gmail.com>
calc/src/calc_fee.rs
Outdated
@@ -98,6 +98,10 @@ impl CalcFee { | |||
spec_version: u32, | |||
) -> Option<CalcFee> { | |||
debug::setup(); | |||
info!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing needs to get fixed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing in that one file calc/pkg/calc_fee.rs
, lgtm though
This PR bumps deps, accounts for changes from polkadot-js/api, and hardens
createCalcFee
which was affected by the polkadot-js changes.Changes introduced by PR:
createCalcFee
AbstractInt
s withtoString(10)
, making sure they are decimal formtoBigInt
on relevantAbstractInt
Call
type before processing argssection
andmethod
instead of the no longer availablesectionName
andmethodName
calc.js
, resulting a big diff there