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

feat: add era to blocks/{blockId} response for extrinsics #685

Merged
merged 15 commits into from
Oct 2, 2021

Conversation

TarikGul
Copy link
Member

@TarikGul TarikGul commented Sep 29, 2021

closes: #486

This adds the era field. @joepetrowski .

The era enum has two possibilities, immortalEra, and mortalEra. Example return values for each:

era: {"immortalEra": "0x00"}
era: {"mortalEra": ["64", "11"]}

Todo:

  • Update e2e-tests
    • kusama
    • polkadot
    • westend (Under the exception 2 tests are failing but they are unrelated to this PR)
  • Update unit tests

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
Comment on lines +1803 to +1805
ex: `"{"mortalEra": ["64", "11"]}"`. The Period is the period of validity from the block hash found in the signing material.
The Phase is the period that this transaction's lifetime begins (and, importantly,
implies which block hash is included in the signature material).
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is a bit dense to parse. Is this correct? The first parameter, 64 in the example, is the Period and indicates the number of blocks for which the extrinsic is valid, starting from the block the extrinsic was signed? But what is the Phase? The wording "The Phase is the period" is confusing to me!

Copy link
Collaborator

@jsdw jsdw Sep 30, 2021

Choose a reason for hiding this comment

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

This always confuses me in the documentation too. I assumed a mortal era was just a block number, and a number of blocks it'd be valid from after that (and the signature material contains the block hash so we can verify that the block numbered is what you'd expect), but I don't think I'm quite right (I'm not sure what a "phase" is)?!

Copy link
Collaborator

Choose a reason for hiding this comment

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

(to build on this; iirc we don't pass a block hash at all in the transaction; we just sign it against a block hash, so you need to know the block number you'll be getting the hash of to verify the signature I think?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the runtime keeps a mapping of BlockNumber => BlockHash for the last BlockHashCount blocks: https://github.com/paritytech/polkadot/blob/v0.9.10/runtime/common/src/lib.rs#L81

The mortality is a number of blocks, but it's encoded in one byte, so it's 2 ** mortality. Front ends abstract this by letting you "choose" a validity period, but if you choose e.g. 50 blocks it would actually be mortal for 64.

immortalEra:
type: string
nullable: true
default: ['0x00']
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, this is the only possible value for the immortal case? So rather than a default, it's just the hard coded value that must be there for Reasons™? If that is correct, can we get rid of it somehow, and have just immortalEra?

Copy link
Member Author

@TarikGul TarikGul Sep 30, 2021

Choose a reason for hiding this comment

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

Yes totally, correct. I agree it should just be immortalEra in this case. A description explaining that the value is hardcoded or will always be '0x00' would possibly suffice?

Copy link
Collaborator

@jsdw jsdw Oct 1, 2021

Choose a reason for hiding this comment

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

One way to represent the return types I guess could be:

era: {
   type: "immortal"
}

or

era: {
   type: "mortal",
   period: 64,
   phase: 12
}

(with a typescript type like { type: "immortal" } | { type: "mortal", period: number, phase: number }. I'm pretty sure that can be represented in openAPI as well but I'd have to remind myself how!)

Copy link
Contributor

Choose a reason for hiding this comment

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

A description explaining that the value is hardcoded or will always be '0x00' would possibly suffice?

That's ok for this PR. Perhaps add an issue to investigate James' suggestion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sweet, so I added a concise description for immortal -> Hardcoded constant '0x00'. I do like the idea of the return type being as @jsdw put it. The one reason I like it is the natural response from substrate has mortal being iterable, and immortal just being a string. Having different data structures in the same response field is a little odd for our circumstances.

I do think for now it might be wise to keep the response as close to substrate as possible. The idea being if anything were to change in substrate (low likely hood), the little logic there is for getting era wont change.

docs/src/openapi-v1.yaml Outdated Show resolved Hide resolved
@TarikGul TarikGul changed the title fix: add eraInfo to blocks/{blockId} response for extrinsics feat: add eraInfo to blocks/{blockId} response for extrinsics Sep 30, 2021
@TarikGul TarikGul changed the title feat: add eraInfo to blocks/{blockId} response for extrinsics feat: add era to blocks/{blockId} response for extrinsics Sep 30, 2021
@TarikGul
Copy link
Member Author

TarikGul commented Oct 2, 2021

Merging

@TarikGul TarikGul merged commit 4362347 into master Oct 2, 2021
@TarikGul TarikGul deleted the tarik-add-erainfo branch October 2, 2021 00:37
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.

add era info in extrinsic
5 participants