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

Runtime version - #256

Merged
gavofyork merged 9 commits into
masterfrom
ark-runtime-ver
Jul 3, 2018
Merged

Runtime version#256
gavofyork merged 9 commits into
masterfrom
ark-runtime-ver

Conversation

@arkpar

Copy link
Copy Markdown
Member

Closes#241

Does not have any performance optimisations.
I've also removed CheckedId because it is now obsoleted by the version check and should be handled by the substrate, rather than polkadot anyway.

@arkpararkpar added the A0-please_review Pull request needs code review. label Jun 27, 2018
Comment threadsubstrate/runtime/version/src/lib.rs Outdated
}

impl<T: Trait> Module<T> {
/// Get runtime verions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo: version?


#[cfg(feature = "std")]
impl RuntimeVersion {
/// Check if this version matches other version for calling into runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we copy descriptions from the issue? They're pretty descriptive there...

Comment threadsubstrate/runtime/version/src/lib.rs Outdated
// You should have received a copy of the GNU General Public License
// along with Substrate Demo. If not, see <http://www.gnu.org/licenses/>.

//! Version module for runtime; Provide a function that returns runtime verion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/verion/version

Comment threadpolkadot/runtime/src/lib.rs Outdated

/// Polkadot runtime version.
pub const VERSION: RuntimeVersion = RuntimeVersion {
spec_name: ver_str!("polkatot"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/polkatot/polkadot

Comment threaddemo/runtime/src/lib.rs Outdated
/// Concrete runtime type used to parameterize the various modules.
pub struct Concrete;

/// Polkadot runtime version.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

demo runtime version?

}

/// Invalid state data.
VersionInvalid {

@gavofyorkgavofyorkJun 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should just assume it's the PoC-1 runtime in this case for now, once we ditch the PoC-1 chain that assumption can be removed and this error can be reinstated.

@gavofyorkgavofyork left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pending the couple of minor changes mentioned.

@gavofyorkgavofyork left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Withdrawing approval until it's clear that the field descriptions of the original issue were faithfully followed.

@gavofyorkgavofyork left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@arkpar

Copy link
Copy Markdown
MemberAuthor

Addressed all issues

@gavofyork

Copy link
Copy Markdown
Member

quoting #241:

Actually, there is one addendum: authorship should require authoring_version and spec_name to be identical. But not spec_version.

@rphmeier

Copy link
Copy Markdown
Contributor

I've also removed CheckedId because it is now obsoleted by the version check and should be handled by the substrate, rather than polkadot anyway.

Why? CheckedId is a type-safe handle to indicate that a block exists. It says "this block ID corresponds to data we know locally", which is a useful tool and IMO is not obsoleted by the version check, which tells you if a known block corresponds to a certain runtime version.

@arkpar

Copy link
Copy Markdown
MemberAuthor

@rphmeier I don't see why it should be checked by the polkadot API and not the substrate. Also, CheckedId does not guarantee anything since the state can be pruned away any time after the check. Does not look safe to me.

@arkpar

Copy link
Copy Markdown
MemberAuthor

@gavofyork Added that last bit

#[cfg(feature = "std")]
#[macro_export]
macro_rules! ver_str {
( $y:expr ) => {{ ::std::borrow::Cow::Borrowed($y) }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

question: why using Cow for std?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

So that it can be defined as const and deserialised from Slicable using the same type.

@gavofyorkgavofyork added A8-looksgood and removed A0-please_review Pull request needs code review. labels Jul 2, 2018
@gavofyork
gavofyork merged commit 710ae51 into masterJul 3, 2018
@gavofyork
gavofyork deleted the ark-runtime-ver branch July 3, 2018 15:44
lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020
…tions (paritytech#256)
* strip out all ICMP code and begin gossip refactor
* validate incoming statements
* message_allowed logic
* compiles
* do reporting and neighbor packet validation
* tests compile
* propagate gossip messages
* test message_allowed
* some more tests
* address grumbles
JoshOrndorff pushed a commit to moonbeam-foundation/substrate that referenced this pull request Apr 21, 2021
* kill processes on exit and sigint
* retyped events and updated api
liuchengxu pushed a commit to autonomys/substrate that referenced this pull request Jun 3, 2022
Updated Farming commands to reflect current snapshot
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@arkpar@gavofyork@rphmeier@pepyakin