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

Add a programs version #1045

Merged
merged 14 commits into from
Sep 16, 2024
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

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?

JesseAbram marked this conversation as resolved.
Show resolved Hide resolved

### Added
- Jumpstart network ([#918](https://github.com/entropyxyz/entropy-core/pull/918))
Expand All @@ -33,6 +34,7 @@ At the moment this project **does not** adhere to
- Attestation pallet ([#1003](https://github.com/entropyxyz/entropy-core/pull/1003))
- Update test CLI for new registration and signing flows ([#1008](https://github.com/entropyxyz/entropy-core/pull/1008))
- Add remove program function to entropy-client ([#1023](https://github.com/entropyxyz/entropy-core/pull/1023))
- Add a programs version ([#1045](https://github.com/entropyxyz/entropy-core/pull/1045))

### Changed
- Fix TSS `AccountId` keys in chainspec ([#993](https://github.com/entropyxyz/entropy-core/pull/993))
Expand Down
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions RELEASE_CHECKLIST.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

JesseAbram marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Update runtime benchmarks
- `cargo build -p entropy --release --features runtime-benchmarks && ./scripts/benchmarks.sh`
- Note: These should ideally be run on [reference hardware](https://wiki.polkadot.network/docs/maintain-guides-how-to-validate-polkadot#reference-hardware) (i.e `c6i.4xlarge` on AWS)
Expand Down
Binary file modified crates/client/entropy_metadata.scale
Binary file not shown.
3 changes: 3 additions & 0 deletions crates/client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ pub async fn sign(
}

/// Store a program on chain and return it's hash
#[allow(clippy::too_many_arguments)]
Copy link
Collaborator

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

#[tracing::instrument(
skip_all,
fields(
Expand All @@ -210,12 +211,14 @@ pub async fn store_program(
configuration_interface: Vec<u8>,
auxiliary_data_interface: Vec<u8>,
oracle_data_pointer: Vec<u8>,
version_number: u8,
) -> Result<<EntropyConfig as Config>::Hash, ClientError> {
let set_program_tx = entropy::tx().programs().set_program(
program,
configuration_interface,
auxiliary_data_interface,
oracle_data_pointer,
version_number,
);
let in_block =
submit_transaction_with_pair(api, rpc, deployer_pair, &set_program_tx, None).await?;
Expand Down
2 changes: 2 additions & 0 deletions crates/client/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ async fn test_store_and_remove_program() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down Expand Up @@ -142,6 +143,7 @@ async fn test_remove_program_reference_counter() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down
3 changes: 3 additions & 0 deletions crates/shared/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

Choose a reason for hiding this comment

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

#1056 opened this

1 change: 1 addition & 0 deletions crates/test-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,4 @@ hex ="0.4.3"
bincode ="1.3.3"
x25519-dalek ="2.0.1"
sp-runtime ={ version="32.0.0", default-features=false }
entropy-shared={ version="0.2.0", path="../shared" }
73 changes: 65 additions & 8 deletions crates/test-cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use entropy_client::{
VERIFYING_KEY_LENGTH,
},
};
pub use entropy_shared::PROGRAM_VERSION_NUMBER;
Copy link
Collaborator

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

use sp_core::{sr25519, Hasher, Pair};
use sp_runtime::traits::BlakeTwo256;
use std::{fs, path::PathBuf};
Expand Down Expand Up @@ -74,6 +75,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>>,
/// A name or mnemonic from which to derive a program modification keypair.
/// This is used to send the register extrinsic so it must be funded
/// If giving a name it must be preceded with "//", eg: "--mnemonic-option //Alice"
Expand Down Expand Up @@ -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>>,
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 not going to be very nice to use, but i can't think of a better 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.

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

/// The mnemonic to use for the call
#[arg(short, long)]
mnemonic_option: Option<String>,
Expand All @@ -121,6 +126,8 @@ enum CliCommand {
config_interface_file: Option<PathBuf>,
/// The path to a file containing the program aux interface (defaults to empty)
aux_data_interface_file: Option<PathBuf>,
/// The version number of the program's runtime you compiled with
program_version_number: Option<u8>,
/// The mnemonic to use for the call
#[arg(short, long)]
mnemonic_option: Option<String>,
Expand Down Expand Up @@ -170,6 +177,7 @@ pub async fn run_command(
program_file_option: Option<PathBuf>,
config_interface_file_option: Option<PathBuf>,
aux_data_interface_file_option: Option<PathBuf>,
program_version_number_option: Option<u8>,
) -> anyhow::Result<String> {
let cli = Cli::parse();

Expand All @@ -183,7 +191,7 @@ pub async fn run_command(
let rpc = get_rpc(&endpoint_addr).await?;

match cli.command {
CliCommand::Register { mnemonic_option, programs } => {
CliCommand::Register { mnemonic_option, programs, program_version_numbers } => {
let mnemonic = if let Some(mnemonic_option) = mnemonic_option {
mnemonic_option
} else {
Expand All @@ -196,9 +204,23 @@ pub async fn run_command(

let mut programs_info = vec![];

for program in programs {
for (i, program) in programs.into_iter().enumerate() {
let program_version_number =
if let Some(ref program_version_numbers) = program_version_numbers {
program_version_numbers[i]
} else {
0u8
};
Copy link
Collaborator

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

programs_info.push(
Program::from_hash_or_filename(&api, &rpc, &program_keypair, program).await?.0,
Program::from_hash_or_filename(
&api,
&rpc,
&program_keypair,
program,
program_version_number,
)
.await?
.0,
);
}

Expand Down Expand Up @@ -248,6 +270,7 @@ pub async fn run_command(
program_file,
config_interface_file,
aux_data_interface_file,
program_version_number,
} => {
let mnemonic = if let Some(mnemonic_option) = mnemonic_option {
mnemonic_option
Expand Down Expand Up @@ -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"),
Copy link
Collaborator

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?

};

let hash = store_program(
&api,
&rpc,
Expand All @@ -284,6 +312,7 @@ pub async fn run_command(
config_interface,
aux_data_interface,
vec![],
program_version_number,
)
.await?;
Ok(format!("Program stored {hash}"))
Expand All @@ -305,7 +334,12 @@ pub async fn run_command(

Ok("Program removed".to_string())
},
CliCommand::UpdatePrograms { signature_verifying_key, mnemonic_option, programs } => {
CliCommand::UpdatePrograms {
signature_verifying_key,
mnemonic_option,
programs,
program_version_numbers,
} => {
let mnemonic = if let Some(mnemonic_option) = mnemonic_option {
mnemonic_option
} else {
Expand All @@ -315,9 +349,24 @@ pub async fn run_command(
println!("Program account: {}", program_keypair.public());

let mut programs_info = Vec::new();
for program in programs {

for (i, program) in programs.into_iter().enumerate() {
let program_version_number =
if let Some(ref program_version_numbers) = program_version_numbers {
program_version_numbers[i]
} else {
0u8
};
Copy link
Collaborator

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

programs_info.push(
Program::from_hash_or_filename(&api, &rpc, &program_keypair, program).await?.0,
Program::from_hash_or_filename(
&api,
&rpc,
&program_keypair,
program,
program_version_number,
)
.await?
.0,
);
}

Expand Down Expand Up @@ -459,6 +508,7 @@ impl Program {
rpc: &LegacyRpcMethods<EntropyConfig>,
keypair: &sr25519::Pair,
hash_or_filename: String,
program_version_number: u8,
) -> anyhow::Result<Self> {
match hex::decode(hash_or_filename.clone()) {
Ok(hash) => {
Expand All @@ -474,10 +524,15 @@ impl Program {
};
Ok(Self::new(H256(hash_32), configuration))
},
Err(_) => Self::from_file(api, rpc, keypair, hash_or_filename).await,
Err(_) => {
Self::from_file(api, rpc, keypair, hash_or_filename, program_version_number)
.await
},
}
},
Err(_) => Self::from_file(api, rpc, keypair, hash_or_filename).await,
Err(_) => {
Self::from_file(api, rpc, keypair, hash_or_filename, program_version_number).await
},
}
}

Expand All @@ -488,6 +543,7 @@ impl Program {
rpc: &LegacyRpcMethods<EntropyConfig>,
keypair: &sr25519::Pair,
filename: String,
program_version_number: u8,
) -> anyhow::Result<Self> {
let program_bytecode = fs::read(&filename)?;

Expand Down Expand Up @@ -526,6 +582,7 @@ impl Program {
config_description,
auxiliary_data_schema,
vec![],
program_version_number,
)
.await
{
Expand Down
2 changes: 1 addition & 1 deletion crates/test-cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use entropy_test_cli::run_command;
#[tokio::main]
async fn main() -> anyhow::Result<()> {
let now = Instant::now();
match run_command(None, None, None).await {
match run_command(None, None, None, None).await {
Ok(output) => {
println!("Success: {}", output.green());
println!("{}", format!("That took {:?}", now.elapsed()).yellow());
Expand Down
1 change: 1 addition & 0 deletions crates/threshold-signature-server/src/helpers/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,7 @@ pub async fn store_program_and_register(
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down
6 changes: 6 additions & 0 deletions crates/threshold-signature-server/src/user/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ async fn test_program_with_config() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down Expand Up @@ -855,6 +856,7 @@ async fn test_compute_hash() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down Expand Up @@ -947,6 +949,7 @@ async fn test_fail_infinite_program() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down Expand Up @@ -1031,6 +1034,7 @@ async fn test_device_key_proxy() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down Expand Up @@ -1183,6 +1187,7 @@ async fn test_faucet() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down Expand Up @@ -1347,6 +1352,7 @@ async fn test_registration_flow() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down
1 change: 1 addition & 0 deletions crates/threshold-signature-server/src/validator/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ async fn test_reshare() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ async fn integration_test_register_and_sign() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down
1 change: 1 addition & 0 deletions crates/threshold-signature-server/tests/sign_eth_tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ async fn integration_test_sign_eth_tx() {
vec![],
vec![],
vec![],
0u8,
)
.await
.unwrap();
Expand Down
Loading
Loading