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(ParasService): adjust endpoint to use historicApi, fix endingOffset bug #735

Merged
merged 17 commits into from
Nov 10, 2021

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Oct 27, 2021

rel: #698

This PR refactors the the ParasService to use a historicApi for querying the chain.

Bug Fix

There was a bug for /experimental/paras/auctions/current?at=8400000 where the incorrect endingOffset was being calculated and passed into auctions.winning which would return the following:

RPC-CORE: getStorage(key: StorageKey, at?: BlockHash): StorageData:: Unable to decode storage auctions.winning:: createType(WinningData):: [Option<(AccountId,ParaId,BalanceOf)>;10]:: Decoded input doesn't match input, received 0x00000000000000016d6f646c70792f6366756e64d10700000000000000000000…0000000000000000000000000000000000000000000000000000000000000000 (88 bytes), created 0x00000000000000016d6f646c70792f6366756e64d107000000000000000000000000000000000000d10700002550b60924ae410100000000000000000000 (62 bytes)
Error: Unable to decode storage auctions.winning:: createType(WinningData):: [Option<(AccountId,ParaId,BalanceOf)>;10]:: Decoded input doesn't match input, received 0x00000000000000016d6f646c70792f6366756e64d10700000000000000000000…0000000000000000000000000000000000000000000000000000000000000000 (88 bytes), created 0x00000000000000016d6f646c70792f6366756e64d107000000000000000000000000000000000000d10700002550b60924ae410100000000000000000000 (62 bytes)

The endingOffset for block 8400000 should be 1478008, but we were receiving 412.

I used the implementation inside of @polkadot/apps in order to fix the current bug.

It also takes into account an edgecase where if the current now block substracted by the offset is less than zero than the leasePeriodIndex will be null.

historicApi: ApiDecoration<'promise'>,
now: BN
): BN {
const leasePeriod = historicApi.consts.slots.leasePeriod as BlockNumber;
return now.div(leasePeriod);
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually need to adjust this due to this PR: paritytech/polkadot#3980

So I think it needs to be (now - lease_offset) / lease_period

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh okay sweet, thanks for the link. If I am seeing this correctly it is coming from this https://github.com/paritytech/polkadot/blob/master/runtime/common/src/crowdloan.rs#L1015

Copy link
Member Author

Choose a reason for hiding this comment

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

@emostov updated if. Could you take a look when you get a chance

Co-authored-by: Zeke Mostov <32168567+emostov@users.noreply.github.com>
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Looks sane; I'm happy if others are! :)

const offset =
(historicApi.consts.slots.leaseOffset as BlockNumber) || BN_ZERO;

return now.sub(offset).div(leasePeriod);
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be updated slightly. There is an edge case where offset > now and we decided to consider this has have no lease period index. In the substrate code this is represented by None. Here is the line paritytech/polkadot@3668966#diff-3e19d8a242311f202d80bcf9998ffdc22ddd933ed340f308276f353f137e2304R444

So basically it says "if now - offset is less than 0, return None". So we need a update for the logic that uses this function to basically say there is no lease period at now and we could add the first lease period start at block offset.

(This logic which got added to the substrate PR a bit late).

Copy link
Contributor

@emostov emostov Nov 6, 2021

Choose a reason for hiding this comment

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

(You can see the changes for this in substrate commit paritytech/polkadot@3668966)

const leasePeriodIndex = currentLeasePeriodIndex + idx;
const leasePeriodIndex = currentLeasePeriodIndex
? currentLeasePeriodIndex.toNumber() + idx
: 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what this should be returning.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be null

@TarikGul TarikGul merged commit ce2ff0b into master Nov 10, 2021
@TarikGul TarikGul deleted the tarik-paras-at branch November 10, 2021 16:31
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.

3 participants