-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add a programs version #1045
Add a programs version #1045
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for everything here.
As i mentioned in the programs PR, the only thing i am unsure about would be to maybe make the version number actually be the major/minor version number of entropy-programs-runtime
. So the current version would be 0.10
rather than 0
.
RELEASE_CHECKLIST.md
Outdated
@@ -14,6 +14,7 @@ tagged as the final release. | |||
- [ ] If an existing call/extrinsic has changed (new pallet index, new call index, parameter changes, | |||
etc.), bump `transaction_version` and bump `spec_version` | |||
- [ ] If you're confused about what to bump, read [this](https://paritytech.github.io/polkadot-sdk/master/sp_version/struct.RuntimeVersion.html) | |||
- [ ] If the programs runtime has been updated increment ```PROGRAM_VERSION_NUMBER``` in ```shared``` crate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -109,6 +112,8 @@ enum CliCommand { | |||
/// interface. If no such file exists, it is assumed the program has no configuration | |||
/// interface. | |||
programs: Vec<String>, | |||
/// Option of version numbers to go with the programs, will default to 0 if None | |||
program_version_numbers: Option<Vec<u8>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this is not going to be very nice to use, but i can't think of a better suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya this took me a bit, was easy until I realized launch a program was possible in all these calls, this was my best solution, we can play with it in the future and change it if it sucks
@@ -76,3 +76,6 @@ pub const SIGNER_THRESHOLD: u8 = 2; | |||
|
|||
/// For testing to line up chain mock data and reshare_test | |||
pub const TEST_RESHARE_BLOCK_NUMBER: u32 = 5; | |||
|
|||
/// Program version number, must be incremented if version number changes | |||
pub const PROGRAM_VERSION_NUMBER: u8 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't yet have a check for this when evaluating the program, but when we do, i think it might make sense for this constant to be exported by entropy-programs-runtime
so that it will always match the runtime version(s) which are actually used.
The argument not to would be if we need this constant in the program's pallet, so that we can refuse to accept programs with an unsupported version number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggled with this decision a little, I came to this for a few reasons, one was it is a PITA the way we are pulling stuff into the templates to make a change and see it reflected. Also we would also have any changes need to be reflected in core before it is usable, and this keeps our updates for releases to core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1056 opened this
Co-authored-by: peg <peg@magmacollective.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a better way to manage versions probably leaning into using the programs-runtime
a bit more, but this is fine for now.
CHANGELOG.md
Outdated
@@ -18,6 +18,7 @@ At the moment this project **does not** adhere to | |||
cleaned up. A lot of storage entries, events, and extrinsics were removed from the `Registry` | |||
pallet. The genesis build config was also removed. Additionally, the `new/user/` HTTP endpoint in | |||
the TSS was removed since it was no longer necessary. | |||
- In [#1045](https://github.com/entropyxyz/entropy-core/pull/1045), `ProgramsInfo` now takes `version_number` to maintain backwards compatibilite if programs runtime is updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you wrap this to 90 characters?
@@ -196,6 +196,7 @@ pub async fn sign( | |||
} | |||
|
|||
/// Store a program on chain and return it's hash | |||
#[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe time to couple the input data
@@ -30,6 +30,7 @@ use entropy_client::{ | |||
VERIFYING_KEY_LENGTH, | |||
}, | |||
}; | |||
pub use entropy_shared::PROGRAM_VERSION_NUMBER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you place this on top of the use
imports? It makes it more clear that this is being re-exported
crates/test-cli/src/lib.rs
Outdated
if let Some(ref program_version_numbers) = program_version_numbers { | ||
program_version_numbers[i] | ||
} else { | ||
0u8 | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably a way to change this to use or_default
, since for a u8
that's 0
anyways
crates/test-cli/src/lib.rs
Outdated
@@ -276,6 +299,11 @@ pub async fn run_command( | |||
)?, | |||
}; | |||
|
|||
let program_version_number = match program_version_number_option { | |||
Some(program_version_number) => program_version_number, | |||
None => program_version_number.expect("No Version number passed"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we just defaulted to 0
in this case, why fail?
crates/test-cli/src/lib.rs
Outdated
if let Some(ref program_version_numbers) = program_version_numbers { | ||
program_version_numbers[i] | ||
} else { | ||
0u8 | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here, can probably be tidied
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com> Co-authored-by: peg <peg@magmacollective.org>
Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>
Adds a version number to programs
Closes #688
Paired with entropyxyz/programs#91