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

added at_latest #900

Merged
merged 3 commits into from
Apr 11, 2023
Merged

Conversation

tadeohepperle
Copy link
Contributor

@tadeohepperle tadeohepperle commented Apr 7, 2023

resolves #899 by providing a simplified API for .at(Option<Hash>) by specialization into .at_latest() and .at(Hash) in various places:

  • storage_client: the at() version does not require async + Result anymore
  • blocks_client: both versions still require async + Result
  • events_client: both versions still require async + Result

Comment on lines 57 to 58
/// Obtain block details given the provided block hash, or the latest block if `None` is
/// provided.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Obtain block details given the provided block hash, or the latest block if `None` is
/// provided.
/// Obtain block details of the latest block hash.

///
/// # Warning
///
/// This call only supports blocks produced since the most recent
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove this warning from the documentation since we are always operating with the latest block.
And therefore, users have no way to utilize an older and incompatible block here.

/// Obtain block details given the provided block hash, or the latest block if `None` is
/// provided.
///
/// # Warning
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Since this function is not publically exposed by subxt, we could also remove the warning message.
I believe we placed that warning to explicitly state in the documentation that older blocks may not be supported, just to set up the user expectations.

@@ -48,6 +48,36 @@ where
/// runtime upgrade. You can attempt to retrieve older blocks,
/// but may run into errors attempting to work with them.
pub fn at(
&self,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also change the documentation of this function to state /// Obtain block details given the block hash.

@lexnv
Copy link
Collaborator

lexnv commented Apr 10, 2023

The code looks good, just a few nits regarding the documentation! 👍

I wonder if we could also remove the need to have the .at() async for the blocks_client and events_client.
Generally, all 3 clients (including storage_client) could have a similar interface, just to avoid user confusion of writing storage_client.at(hash); and blocks_client.at(hash).await.

For the blocks_client part, we call Substrate to fetch the Block::Header, which keeps us from removing the async of the .at(). One idea here would be to fetch Block::Header lazily since we already require an async to fetch events, body, and runtime API.


/// Obtain block details given the provided block hash, or the latest block if `None` is
/// provided.
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

I would like some consistency here w.r.t. to inline here as this method is marked as "inline" but the wrappers on top of this are not.

@lexnv what do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@niklasad1 I wanted to keep consistency to how it was before. Before, the pub method was also not inlined. Now the inline will just generate two pub functions at() and at_latest()that are both not inlined (like before).

Copy link
Member

Choose a reason for hiding this comment

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

I see, makes sense but let's hear what @lexnv thinks :)

Copy link
Collaborator

@jsdw jsdw Apr 11, 2023

Choose a reason for hiding this comment

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

I wouldn't have the #[inline] here myself (if anything I'd have it on at() and at_latest() because they are trivially inlineable at the call site and would have a similar effect with less code duplication).

But more to the point, at() no longer needs to be async, so I think that this implementation should be split up and the relevant part of it put in each of at() and at_latest(), so that only the latter needs to be declared async now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see; it would have to be async anyway! In that case let';s just remove the #[inline] (if anything it's be best on at() and at_latest() I think) and this looks good to me!

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

I like it and makes the code more readable, all good.

Some comments about "inline attributes" these are in general added by the rust compiler but if we want to use them in these functions we should use them in the entire call tree. not just in the leaf methods.

/// runtime upgrade. You can attempt to retrieve events from older blocks,
/// but may run into errors attempting to work with them.
#[inline]
fn at_optional_block_hash(
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, let's put this code directly into at() and at_latest() so that the former no longer needs to be async, and no need for #[inline] :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see; both have to be async anyway actually!

In this case, let's just remove the #[inline] and it's all good :)

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.

A couple of comments to tweak per Alex's comments, and I'd remove the #[inline]s, but otherwise looks great!

@tadeohepperle tadeohepperle merged commit 3b9fd72 into master Apr 11, 2023
@tadeohepperle tadeohepperle deleted the tadeohepperle-add-at-latest-in-various-places branch April 11, 2023 11:07
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 at_latest() to use instead of .at(None) in various places.
4 participants