Prepare test CLI for use in Programs repo #856

JesseAbram · 2024-05-27T22:42:19Z

This allows for it to be used in programs repo to allow us to maintain only one cli. This also adds a few extras I think are nice

ameba23

Fine by me. Im not sure how you will be able to use this in a different way than it is used here with only the run_command fn being public. But i guess if other stuff needs to be public you will figure that out at the point of needing it.

main.rs should have a license header.

HCastano · 2024-05-28T07:43:33Z

@JesseAbram Do you mind elaborating on the motivation a bit here?

We can probably build some features for interacting with programs here first before
making the decision to split this out. This will also give us a bit of a more informed
decision in terms of where to make the split, what things to make public, etc.

It's also unclear from the diff or commits what "few extras" you added, do you mind
pointing those out?

JesseAbram · 2024-05-28T13:45:11Z

Fine by me. Im not sure how you will be able to use this in a different way than it is used here with only the run_command fn being public. But i guess if other stuff needs to be public you will figure that out at the point of needing it.

main.rs should have a license header.

well really all we need is store program, and we can make changes to it to fit the programs cli better, im thinking we pass an option into run command that can be passed down the line for program file (so we can make it easier to pass compiled files), other than that everything works the same as it is now

JesseAbram · 2024-05-28T13:47:46Z

@JesseAbram Do you mind elaborating on the motivation a bit here?

like my comment says we currently maintain two cli that do the same thing, one here and one in the programs repo, we can now merge them because core was opened. cli

We can probably build some features for interacting with programs here first before making the decision to split this out. This will also give us a bit of a more informed decision in terms of where to make the split, what things to make public, etc.

we do, we have store program, and that is really all we need for the programs repo

It's also unclear from the diff or commits what "few extras" you added, do you mind pointing those out?

Handling of the mnemonic also now includes allowing it to be in a .env instead of passed in. As well we can pass in a program aux data and config to the run_command function so we can handle getting the data automatically from the template

HCastano · 2024-05-28T15:51:11Z

@JesseAbram Do you mind elaborating on the motivation a bit here?

like my comment says we currently maintain two cli that do the same thing, one here and one in the programs repo, we can now merge them because core was opened. cli

To go deeper on this train of though, why not maintain the programs related functionality in this CLI instead of pulling all the "core" stuff into the Programs CLI? That way we can still maintain one CLI.

We can probably build some features for interacting with programs here first before making the decision to split this out. This will also give us a bit of a more informed decision in terms of where to make the split, what things to make public, etc.

we do, we have store program, and that is really all we need for the programs repo

Similar to my point above, we make the test CLI the only tool for now.

It's also unclear from the diff or commits what "few extras" you added, do you mind pointing those out?

Handling of the mnemonic also now includes allowing it to be in a .env instead of passed in. As well we can pass in a program aux data and config to the run_command function so we can handle getting the data automatically from the template

Okay cool. Next time do you mind splitting that out into its own commit(s) to make it easier for review 🙏

JesseAbram · 2024-05-28T16:00:03Z

@JesseAbram Do you mind elaborating on the motivation a bit here?

like my comment says we currently maintain two cli that do the same thing, one here and one in the programs repo, we can now merge them because core was opened. cli

To go deeper on this train of though, why not maintain the programs related functionality in this CLI instead of pulling all the "core" stuff into the Programs CLI? That way we can still maintain one CLI.

a few reasons, one is because the testcli serves as an internal testing tool and forces us to maintain it by deafult in core, as in if we make a change to core we have to fix the cli instead of maybe remembering and seeing we broke it later down the line. As well the cli in the programs repo requires really only one of the functions so I mean it would be weird to put all the extra stuff we need. Down the road I hope the cli will also be used by validators if they need to do functions on their validator, so having it right there in the core makes sense to me. Also I mean it isn't that much stuff, I don't see the issue in pulling it into the programs repo

We can probably build some features for interacting with programs here first before making the decision to split this out. This will also give us a bit of a more informed decision in terms of where to make the split, what things to make public, etc.

we do, we have store program, and that is really all we need for the programs repo

Similar to my point above, we make the test CLI the only tool for now.

It's also unclear from the diff or commits what "few extras" you added, do you mind pointing those out?

Handling of the mnemonic also now includes allowing it to be in a .env instead of passed in. As well we can pass in a program aux data and config to the run_command function so we can handle getting the data automatically from the template

Okay cool. Next time do you mind splitting that out into its own commit(s) to make it easier for review 🙏
can do

HCastano · 2024-05-28T16:11:39Z

a few reasons, one is because the testcli serves as an internal testing tool and forces us to maintain it by deafult in core, as in if we make a change to core we have to fix the cli instead of maybe remembering and seeing we broke it later down the line.

Yeah we're on the same page there, the test CLI should remain where it is for development purposes.

As well the cli in the programs repo requires really only one of the functions so I mean it would be weird to put all the extra stuff we need. Down the road I hope the cli will also be used by validators if they need to do functions on their validator, so having it right there in the core makes sense to me. Also I mean it isn't that much stuff, I don't see the issue in pulling it into the programs repo

There is actually quite a bit of stuff that gets pulled in:

Since the entropy-test-cli depends on the entropy-tss package we pull in ~900 crates. So now we'd also be pulling those into the Programs CLI.

To clarify what I'm saying: I don't think we need to move the test CLI elsewhere. Instead we should consider moving the Programs related functionality here (assuming it's not a lot) and then encourage Program developers to install the entropy-test-cli from crates.io and work that way. Wdyt?

JesseAbram · 2024-05-28T16:16:48Z

a few reasons, one is because the testcli serves as an internal testing tool and forces us to maintain it by deafult in core, as in if we make a change to core we have to fix the cli instead of maybe remembering and seeing we broke it later down the line.

Yeah we're on the same page there, the test CLI should remain where it is for development purposes.

As well the cli in the programs repo requires really only one of the functions so I mean it would be weird to put all the extra stuff we need. Down the road I hope the cli will also be used by validators if they need to do functions on their validator, so having it right there in the core makes sense to me. Also I mean it isn't that much stuff, I don't see the issue in pulling it into the programs repo

There is actually quite a bit of stuff that gets pulled in:
Since the `entropy-test-cli` depends on the `entropy-tss` package we pull in ~900 crates. So now we'd also be pulling those into the Programs CLI.
To clarify what I'm saying: I don't think we need to move the test CLI elsewhere. Instead we should consider moving the Programs related functionality here (assuming it's not a lot) and then encourage Program developers to install the entropy-test-cli from crates.io and work that way. Wdyt?

sorry that stuff it already here, and I see what you mean but there are some helpers we can add that really greese the wheels here https://github.com/entropyxyz/programs/pull/83/files#diff-952b6817e3c56099926b7e90cfc8823a75ddfcfc48442217dc6d00e93dc99790R10-R14

plus I mean that extra stuff only gets built when you build the CLI, and stays local (not hosting it anywhere) so in the end you will have to build it anyways, personally I weigh the ease of being able to pull files directly and use the .env config (im not sure it will work if we don;t have it in the root, I could be wrong here tho) as worth it

ameba23 · 2024-05-29T10:51:16Z

Since the entropy-test-cli depends on the entropy-tss package we pull in ~900 crates. So now we'd also be pulling those into the Programs CLI.

I should probably fix this. We only need the dependency entropy-testing-utils (which has entropy-tss as a dependency) for having the example barebones program as a default program. We could probably save a massive about of extra compiling if we put this somewhere common or the other stuff behind a feature flag.

Also worth noting that the CLI is just a thin wrapper around entropy-client which has eg: a store_program function: https://docs.rs/entropy-client/latest/entropy_client/client/fn.store_program.html - and doesn't have too many dependencies.

ameba23 · 2024-05-29T11:30:08Z

@JesseAbram having actually tried this out, i am not happy with needing to give the mnemonic as an optional argument when registering or updating programs, as in the context of this CLI it is not optional, which makes this confusing. The readme would also need to be updated. I missed this in the big diff.

If you want to be able to read it from an environment variable, you should either read the environment variable inside this CLI, or factor out the bits you need from run_command rather than modifying it.

I think if you need stuff from this CLI for another CLI, you should be to get those things without changing the behavior of this CLI.

JesseAbram · 2024-05-29T14:40:25Z

@JesseAbram having actually tried this out, i am not happy with needing to give the mnemonic as an optional argument when registering or updating programs, as in the context of this CLI it is not optional, which makes this confusing. The readme would also need to be updated. I missed this in the big diff.

If you want to be able to read it from an environment variable, you should either read the environment variable inside this CLI, or factor out the bits you need from run_command rather than modifying it.

I think if you need stuff from this CLI for another CLI, you should be to get those things without changing the behavior of this CLI.

I mean I do, you can add a .env to the repo, I think the .env flow is better for use longterm, but also allows for a mnemonic to be passed in, I default it to //Alice but I can always error it out if none is passed, keeping essentially the same flow

ameba23 · 2024-05-29T15:49:57Z

crates/test-cli/src/lib.rs

@@ -179,7 +179,7 @@ pub async fn run_command(
        std::env::var("ENTROPY_DEVNET").unwrap_or("ws://localhost:9944".to_string())
    });

-    let passed_mnemonic = std::env::var("DEPLOYER_MNEMONIC").unwrap_or("//Alice".to_string());
+    let passed_mnemonic = std::env::var("DEPLOYER_MNEMONIC");


Ok got it this is good, i missed this before and thought passed_mnemonic was one of the arguments to run_command.

ameba23 · 2024-05-29T15:58:14Z

crates/test-cli/README.md


-`entropy-test-cli register Alice public template_barebones.wasm`
+`entropy-test-cli register public template_barebones.wasm template_barebones_config_data template_barebones_aux_data //Alice`


I don't think this will work. Mnemonic is not a positional argument, its an #[arg(short, long)] which is given with --mnemonic-option //Alice or -m //Alice.

ameba23 · 2024-05-29T15:59:17Z

crates/test-cli/README.md

@@ -129,7 +116,7 @@ a program you can use the `store-program` command.
 You need to give the account which will store the program, and the path to a program binary file you
 wish to store, for example:

-`entropy-test-cli store-program Alice ./crates/testing-utils/example_barebones_with_auxilary.wasm`
+`entropy-test-cli store-program ./crates/testing-utils/example_barebones_with_auxilary.wasm //Alice`


turn cli into lib

4ef15f7

JesseAbram requested review from ameba23 and HCastano May 27, 2024 22:42

ameba23 approved these changes May 28, 2024

View reviewed changes

add changes to store program

78bc9d6

HCastano changed the title ~~Turn cli into lib~~ Prepare test CLI for use in Programs repo May 28, 2024

add licence header

7f711f4

ameba23 mentioned this pull request May 29, 2024

Test CLI store program command no longer defaults to the barebones program #860

Closed

JesseAbram added 2 commits May 29, 2024 10:43

remove testing utils from cargo toml

efc2507

update menmoic handling

2933cdc

JesseAbram requested a review from ameba23 May 29, 2024 15:06

update readme

254ce55

ameba23 approved these changes May 29, 2024

View reviewed changes

JesseAbram added 2 commits May 29, 2024 12:08

fix reademe mnemonic

93135ed

add changelog

a1865d4

JesseAbram merged commit 52d1723 into master May 29, 2024
13 checks passed

JesseAbram deleted the prepare-test-cli-for-use-in-programs-repo branch May 29, 2024 17:13

HCastano added Breaks API labels Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare test CLI for use in Programs repo #856

Prepare test CLI for use in Programs repo #856

JesseAbram commented May 27, 2024

ameba23 left a comment

HCastano commented May 28, 2024

JesseAbram commented May 28, 2024

JesseAbram commented May 28, 2024 •

edited

Loading

HCastano commented May 28, 2024

JesseAbram commented May 28, 2024

HCastano commented May 28, 2024

JesseAbram commented May 28, 2024

ameba23 commented May 29, 2024

ameba23 commented May 29, 2024

JesseAbram commented May 29, 2024

ameba23 May 29, 2024

ameba23 May 29, 2024

JesseAbram May 29, 2024

ameba23 May 29, 2024


		`entropy-test-cli register Alice public template_barebones.wasm`
		`entropy-test-cli register public template_barebones.wasm template_barebones_config_data template_barebones_aux_data //Alice`

Prepare test CLI for use in Programs repo #856

Prepare test CLI for use in Programs repo #856

Conversation

JesseAbram commented May 27, 2024

ameba23 left a comment

Choose a reason for hiding this comment

HCastano commented May 28, 2024

JesseAbram commented May 28, 2024

JesseAbram commented May 28, 2024 • edited Loading

HCastano commented May 28, 2024

JesseAbram commented May 28, 2024

HCastano commented May 28, 2024

JesseAbram commented May 28, 2024

ameba23 commented May 29, 2024

ameba23 commented May 29, 2024

JesseAbram commented May 29, 2024

ameba23 May 29, 2024

Choose a reason for hiding this comment

ameba23 May 29, 2024

Choose a reason for hiding this comment

JesseAbram May 29, 2024

Choose a reason for hiding this comment

ameba23 May 29, 2024

Choose a reason for hiding this comment

JesseAbram commented May 28, 2024 •

edited

Loading