Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Relax types on DigestItemRef, such that byte slices can be used in addition to vector references #10536

Merged
merged 5 commits into from
Jan 20, 2022

Conversation

nazar-pc
Copy link
Contributor

The goal here is to use DigestItemRef with frame_support::traits::FindAuthor, so I can turn (ConsensusEngineId, &'a [u8]) into DigestItemRef without extra allocation:

pub trait FindAuthor<Author> {
	/// Find the author of a block based on the pre-runtime digests.
	fn find_author<'a, I>(digests: I) -> Option<Author>
	where
		I: 'a + IntoIterator<Item = (ConsensusEngineId, &'a [u8])>;
}

Ideally, I'd like to see FindAuthor having &'a DigestItem instead of (ConsensusEngineId, &'a [u8]) if that is OK (this PR is still useful though).

On a similar note DigestItemRef should really use ConsensusEngineId instead of &'a ConsensusEngineId because reference will be more expensive to copy than the data itself.

Technically this PR introduces a breaking change to the API, but it is extremely unlikely to be an issue in practice as the source of that iterator is almost certainly Vec<DigestItem>.

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Looks good, beside the version bump.

primitives/runtime/Cargo.toml Show resolved Hide resolved
@bkchr bkchr added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Dec 22, 2021
…-ref

# Conflicts:
#	client/chain-spec/Cargo.toml
#	client/finality-grandpa/Cargo.toml
#	test-utils/runtime/transaction-pool/Cargo.toml
@bkchr
Copy link
Member

bkchr commented Jan 15, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Checks failed for f3fe72f

@bkchr
Copy link
Member

bkchr commented Jan 15, 2022

@nazar-pc Can you add a polkadot companion?

@nazar-pc
Copy link
Contributor Author

This PR is meant to be fully compatible with it and indeed I see no issues locally 🤷‍♂️
CI errors do not seem related to this PR, I pulled master again just in case.

@bkchr
Copy link
Member

bkchr commented Jan 18, 2022

This didn't helped.

@nazar-pc
Copy link
Contributor Author

I think there is something related to lock file that forces it to load incorrect version of the package whose version was updated, but I can't reproduce the issue locally.

@bkchr
Copy link
Member

bkchr commented Jan 18, 2022

Can you merge master again. I will then merge this pr manually.

…-ref

# Conflicts:
#	test-utils/client/Cargo.toml
@bkchr bkchr merged commit 823c253 into paritytech:master Jan 20, 2022
@nazar-pc nazar-pc deleted the relax-digest-item-ref branch January 20, 2022 18:36
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
…addition to vector references (paritytech#10536)

* Relax types on `DigestItemRef`, such that byte slices can be used in addition to vector references

* Apply clippy suggestions
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…addition to vector references (paritytech#10536)

* Relax types on `DigestItemRef`, such that byte slices can be used in addition to vector references

* Apply clippy suggestions
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants