feat: support multiple commands and multiple binaries in aliases

[lima-limon-inc]

Closes #129

This PR makes adds the following functionalities:

• Channels can also have Aliases
• Aliases can now contain a series of commands instead of just one.

Both of these features were added in order to support the miden mint command (described in more detail here: 0xMiden/miden-faucet#192) which requires a succession of command from different binaries to be executed.

To achieve this, the following changes were made:

• MidenArgument::Alias now contains a Vec<CLICommand> instead of a single CLICommand. Additionally, the Component field was removed, in order to support multi-binary aliases.
• CliCommand::Executable now requires the following argument to be of type CliCommand::Verbatim with the name of the component that needs to be executed.
  • This does mean that component specific aliases now require to redundantly add their own name in the aliases, like so.
  • Additionally, it does mean that a component could technically use a different component's binary in its alias. While not ideal.
• Added CliCommand::PositionalArgument, which can be used to pass specific positional argument used in the CLI to the underlying binaries. This mainly intended for multi-command aliases, like so:

      "aliases": {
          "mint": [
              ["executable", "faucet-client", "mint", "-a", 0],
              ["executable", "client", "sync"],
              ["executable", "client", "consume-notes", "-a", 0]
          ]
      }

This means that when a user calls the miden mint alias, the first argument they pass to miden mint will be passed to faucet-client and client executables.

[lima-limon-inc]

I'm not too fond of Executable and VarPath depending implicitly on the following CliCommand on the manifest.
I believe these two could be refactored so that all the required arguments are on the same enum variant.
Like so:

pub enum CliCommand {
    (...)
    Executable { component_name: String },
    (...)

This would change the way CliCommand is serialized, but I think encapsulating all the required component into a single variant would a win in the long run.

[lima-limon-inc]

Sidenote: Since this changes the way the Manifest is serialized, it is expected for this test to fail; since it uses the published manifest.

[lima-limon-inc]

I removed this test since installing the stable toolchain is already done in .github/workflows/integration.yml.

Additionally, the integration tests are only run if the unit tests pass. So currently, this test is only blocking the integration test suite; without providing additional functionality.

[Keinberger]

Hey @lima-limon-inc , thanks for working on this! I was actually also working on implementing a solution to #129 in this PR: #132

FYI, I think it might make sense to compare the two approaches. On PR #132 , I moved the aliases outside of the individual components struct, to a "aliases" struct that sits at the top-level of each channel (which caused significantly more code changes). The future mint command will execute commands form both the miden-faucet and miden-client. Keeping the aliases inside of the individual executables would make that less ideal, as you also pointed out in your PR description.

Furthermore, I think we need better coordination on who is working on what for midenup related stuff, since we were both working on the same thing😅

[lima-limon-inc]

Hi there @Keinberger !

FYI, I think it might make sense to compare the two approaches.

Agreed! I looked at your PR and I believe our approaches are more similar than they are dissimilar.

On PR #132 , I moved the aliases outside of the individual components struct, to a "aliases" struct that sits at the top-level of each channel (which caused significantly more code changes).

I also added this channel-wide aliases in my PR. I believe having aliases which encompass multiple binaries makes these "channel wide aliases" a necessity.

I think the main difference in approach between both PRs is that #132 removes component specific aliases, whilst this PR keeps them.

the future mint command will execute commands form both the miden-faucet and miden-client. Keeping the aliases inside of the individual executables would make that less ideal, as you also pointed out in your PR description

I believe keeping the concept of "component specific aliases" is worthwhile. Were the CLI of a component to change, it would only be that component's responsibility to update the corresponding manifest entries.
This does depend on the approach we take with regards to #126, i.e., how the manifest is generated in the future; but I imagine that, broadly speaking, it will involve delegating to each component how its manifest entry is generated.

On the other, aliases that correspond to multiple binaries, like miden mint, will probably still need special treatment (most likely in midenup repository itself). But I do think that this separation will ease manifest maintainability looking forward, since only these multi-binary aliases would be managed by midenup itself..
Would love to hear your take on this @Keinberger. Additionally, just so we are on the same page, this distinction would only exist in the code, from the user's perspective, all the aliases function the same, see this snippet.

Furthermore, I think we need better coordination on who is working on what for midenup related stuff, since we were both working on the same thing😅

Totally, I apologize for any inconvenience this might have caused.

Having said this, I'll be working on a slight refactor of CliCommand (on a separate PR in order for the core functionality to get merged), tackling this comment: #130 (comment).

I'm not too certain of going with this table-like approach, since that might limit midenup's usability with tools that are not part of miden ecosystem itself, (things like cargo for instance, like in this, now closed, PR).

Edit: The PR in question, editing CliCommand serialization is here:#133

[bitwalker]

@lima-limon-inc @Keinberger I would like to better understand why this feature is necessary in the first place. It isn't clear to me why miden-faucet is separate from miden-client, when it would seem that to do something useful here, i.e. mint, both executables are required to cooperate anyway. In other words, is there a good reason why miden-client doesn't provide a mint command that internally uses the miden-faucet crate to implement that behavior?

midenup/miden are supposed to be very thin, very rarely updated tools - the underlying components are expected to provide the actual meat on the bone. The moment we start trying to encode complex workflows in the channel manifest, we're introducing a lot of ways that switching between toolchain versions can break. The sort of complexity implemented here should really be pushed down to miden-client IMO.

If there is a compelling reason why miden-faucet cannot be integrated into miden-client, then we might have to go down this path, but I really want to avoid that if at all possible.

[igamigo]

Furthermore, I think we need better coordination on who is working on what for midenup related stuff, since we were both working on the same thing😅

In this case I was expecting @lima-limon-inc to work on this because he self-assigned the related issue as soon as it was created AFAIK, but more generally I agree! The fact that it's still not clear where the feature should be implemented is also related.

@lima-limon-inc @Keinberger I would like to better understand why this feature is necessary in the first place. It isn't clear to me why miden-faucet is separate from miden-client, when it would seem that to do something useful here, i.e. mint, both executables are required to cooperate anyway. In other words, is there a good reason why miden-client doesn't provide a mint command that internally uses the miden-faucet crate to implement that behavior?

I think some of the rationale can be found in the closed client PR (cc @mmagician).

[bitwalker]

I think some of the rationale can be found in the closed client PR (cc @mmagician).

Based on my reading of that comment, it would seem that the inverse could be done instead then, implement mint under miden-faucet, which internally uses the miden-client crate as necessary. Essentially miden-faucet ends up being a specialized miden-client.

In general though, a key design constraint is that miden isn't actually it's own binary, so we can't implement arbitrary functionality in miden (and shouldn't). Instead, it's expected that the underlying components provide that behavior, and miden just surfaces it as a unified CLI.

If we determine that we really want miden to provide this sort of meta scripting capability, then I would argue that we should make the miden CLI itself a separate executable from midenup, installed as a toolchain component. Most of the stuff we've forced into the channel manifest around aliases and such would just go away, or be built in to miden directly, and midenup would retain its simplicity and role as toolchain manager.

Either way, I don't think the direction in this PR or in #132 are the way forward. It strikes me that if the need for this kind of capability is more pervasive than just the mint command, then we probably need to split miden out as described above, so it can just be as arbitrarily complex as we want.

[Keinberger]

@lima-limon-inc

I believe keeping the concept of "component specific aliases" is worthwhile. Were the CLI of a component to change, it would only be that component's responsibility to update the corresponding manifest entries.
This does depend on the approach we take with regards to #126, i.e., how the manifest is generated in the future; but I imagine that, broadly speaking, it will involve delegating to each component how its manifest entry is generated.
On the other, aliases that correspond to multiple binaries, like miden mint, will probably still need special treatment (most likely in midenup repository itself). But I do think that this separation will ease manifest maintainability looking forward, since only these multi-binary aliases would be managed by midenup itself..
Would love to hear your take on this @Keinberger. Additionally, just so we are on the same page, this distinction would only exist in the code, from the user's perspective, all the aliases function the same, see this snippet.

Thanks for explaining, I agree that it is worthwhile to keep the component-centric aliases. Nevertheless, one suggestion that I'd like to make would be to rename the "key" for channel-wide aliases in the manifest file. Otherwise I fear it's confusing as to why aliases are defined channel-wide, and component-specific.

Totally, I apologize for any inconvenience this might have caused.
Having said this, I'll be working on a slight refactor of CliCommand (on a separate PR in order for the core functionality to get merged), tackling this comment: #130 (comment).
I'm not too certain of going with this table-like approach, since that might limit midenup's usability with tools that are not part of miden ecosystem itself, (things like cargo for instance, like in #118).
Edit: The PR in question, editing CliCommand serialization is here:#133

Sounds good, no worries. I think we can utilize our internal Slack channel for better communication. Also, I missed that you self-assigned this issue, so that one is on me.

[Keinberger]

@bitwalker

@lima-limon-inc @Keinberger I would like to better understand why this feature is necessary in the first place. It isn't clear to me why miden-faucet is separate from miden-client, when it would seem that to do something useful here, i.e. mint, both executables are required to cooperate anyway. In other words, is there a good reason why miden-client doesn't provide a mint command that internally uses the miden-faucet crate to implement that behavior?

As Ignacio already pointed out, this was discussed on the original PR on miden-client (I wanted to implement it there).

I would also like to link this comment by Santiago, in which he explained that the inclusion of the mint command of the miden-client directory itself would break the client's dependency tree: 0xMiden/miden-client#1508 (review)

Based on my reading of that comment, it would seem that the inverse could be done instead then, implement mint under miden-faucet, which internally uses the miden-client crate as necessary. Essentially miden-faucet ends up being a specialized miden-client.

Here my comment on your thought: Per se, I do agree that we can do that. Nevertheless, I think it's important to point out that this would break the miden-faucet component "uniqueness". Here's what I mean:

Right now, miden-faucet is just a repository that contains logic for spinning up a faucet backend (and frontend) for issuing tokens on any Miden network. It does not integrate with the client CLI in any way for that.

If we were to integrate the full logic of the mint command there (i.e. integrating and depending on the client CLI), the faucet itself would become sort of an extension of the "client", which I'm not sure we want. Two reasons for that:

• The faucet itself wouldn't be a distinct component for running a asset-issuance backend anymore, as it would directly depend on the client CLI
• Users would need to correctly configure and install their miden-client instance before using the faucet's mint command. I think this feels counterintuitive, requiring setup of an external component to use what appears to be a faucet feature.

On the other hand, if we consider that the mint command lives on midenup, I do agree that this would bring more complex logic to the midenup toolchain repository, nevertheless, we do not have the same problem as with the faucet approach. midenup installs all Miden CLI tools natively, which means that users are guaranteed to have install both the faucet CLI and the client CLI before running mint (solves external dependency set up). Also, from a user perspective this is more intuitive.

I'm open to either direction, but wanted to surface these trade-offs. Would be great to hear other people's perspective on this too!

[mmagician]

Right now, miden-faucet is just a repository that contains logic for spinning up a faucet backend (and frontend) for issuing tokens on any Miden network. It does not integrate with the client CLI in any way for that.

Not with the CLI, but miden-faucet libs/bins have miden-client as their dependency.

Currently, the miden-faucet-client binary for requesting/minting tokens uses miden-client as well, but AFAICS these are only for imports, not for functionality - so it could probably be refactored to use miden-base (not sure if that's a good idea, just saying), or eventually miden-client-core .

---

I think we're conflating two concepts in the faucet discussions: miden-faucet repo used to be an application for a faucet owner to issue assets (+ frontend). We've now got a PR to also add faucet user functionality to the miden-faucet repo as miden-faucet-client.

The latter can be refactored to remove its dependency on miden-client (see above). And midenup need not depend on miden-faucet-operator. So we'd end up with midenup only depending on miden-faucet-client binary, which we can think of essentially as an http client - this should be fine right?

[igamigo]

Either way, I don't think the direction in this PR or in #132 are the way forward. It strikes me that if the need for this kind of capability is more pervasive than just the mint command, then we probably need to split miden out as described above, so it can just be as arbitrarily complex as we want.

Yes, I agree this is probably a pattern that would repeat in the future, and that splitting features out into a different tool could work out here (I couldn't find it just now but I think I suggested a similar approach when the implementation was going to live on the client repo).
Although in general, I'm not sure I fully understand how multi-binary command aliases could introduce new ways for the toolchain switching to break. As long as commands are their interfaces are tied to the correct versions, should it not be fine?

Based on my reading of that comment, it would seem that the inverse could be done instead then, implement mint under miden-faucet, which internally uses the miden-client crate as necessary. Essentially miden-faucet ends up being a specialized miden-client.

Do you mean doing something like

// Request note from the faucet as it's currently done in https://github.com/0xMiden/miden-faucet/pull/196

// This does not currently exist but essentially it would load a config in the same way as the CLI does and 
// init a client
let client = Client::from_system_user_config();
let req = TransactionRequest::builder().consume_notes(/* recently_minted_note */).build()?;

client.submit_new_transaction(req, target_account_id)?;

If so, I think this would also work. As you mentioned, the faucet user binary becomes a specialized client which just implements functionality for a specific subset of transactions (consuming notes from the faucet with a specific account). I may have misunderstood @Keinberger's reservations but I'm not sure they apply here; as @mmagician mentioned, the backend already relies on the client and additionally I don't think there would be a need to config the client instance necessarily (not in a way that it wasn't necessary before if you wanted to get assets into an account managed by yourself).

[bitwalker]

Sorry it has taken me so long to follow up here, I've been pretty swamped as of late 😅.

Do you mean doing something like ... If so, I think this would also work.

Yes, exactly. Since the faucet already depends on the client, it would seem natural then to have the mint command live there, and have it handle the relationship between the two crates.

I don't think there would be a need to config the client instance necessarily (not in a way that it wasn't necessary before if you wanted to get assets into an account managed by yourself).

Agreed, and in particular, the client would have already been configured via midenup presumably (and/or by a prior use of the client via miden), and the faucet can simply piggy-back off of that configuration. I can't recall off the top of my head if the client has been set up to auto-initialize its state/configuration, or prompt the user to do so, but I'd expect that if the faucet calls into the client, then it could do the same there as well. If that initialization step is still being handled by midenup, then as long as the faucet component depends on the client component, then initialization of the client is guaranteed to happen before the faucet gets used anyway.

Yes, I agree this is probably a pattern that would repeat in the future, and that splitting features out into a different tool could work out here

Fair. I think I'd like to wait and see another example of that pattern before we attempt to solve for it in midenup/miden though - not least because it will be clearer what the pattern actually is. For now, we only have this one scenario, that has a viable alternative implementation path, so it doesn't make for a particularly compelling use case IMO.

Although in general, I'm not sure I fully understand how multi-binary command aliases could introduce new ways for the toolchain switching to break. As long as commands are their interfaces are tied to the correct versions, should it not be fine?

I'm more concerned about breaking changes to midenup itself and its manifest format, not the components themselves. By encoding more toolchain features/command handling in midenup (and necessarily then making its manifest more of a DSL), we significantly increase the likelihood of breaking and/or backwards-incompatible changes to midenup (and/or its manifest). My fear is that this would make toolchain upgrades and switching between older and newer toolchains take a dependency on specific version ranges of midenup itself, which defeats its purpose.

Consider rustup, which was the primary inspiration for midenup and its implementation - even with all the changes to Rust itself, and its toolchain components, I'm not aware of any requirement that you be running a specific version of rustup regardless of what version of Rust you want to work with (at least in the last several years). New rustup releases are very rare, because the actual meat of Rust is handled by the toolchain components, not rustup.

As I noted though, miden is part of midenup itself, and it may turn out that due to one reason or another, we'll need to split it out from midenup and treat it as its own component (likely one that is required/always installed). That comes with some downsides of its own, but I think that would be a better road to go down (if we need to), than to compromise the stability/reliability of midenup's toolchain management role.

[Keinberger]

Okay, thanks everyone for sharing your thoughts!

I want to summarize everything here so we can align on an approach moving forward:

👉 From what it seems, we want to move the note consumption functionality to the miden-faucet repository (i.e., the miden-faucet-client crate) directly.

I'm fine with that. For the record, I'm also fine with the other approach that was discussed, i.e., separating miden from midenup.

There is one thing I would like to mention regarding moving the note consumption to the faucet repository:
Moving this logic to the faucet repository would mean that the miden-faucet-client crate will directly use the miden-client crate as a library dependency to handle note consumption (e.g., syncing and consuming notes programmatically). For this to work correctly, we should implement a Client::from_system_user_config() function (as @igamigo posted in a previous message) in the miden-client crate that replicates the same logic the client CLI uses for determining: 1) the correct config TOML file and 2) the correct keystore directory.

This is important to ensure that the faucet uses the same configuration as the client CLI would (which is the intended user behavior). Since the client specifies how to determine the correct .miden directory and its corresponding config TOML and keystore directory files, I think this function should live in the client repository directly. I want to mention this here so that everyone is aware, in case there are any objections to this approach.

Please let me know if everyone is fine with moving the functionality to the faucet repo and implementing this helper function in the client repository!

[bitwalker]

👉 From what it seems, we want to move the note consumption functionality to the miden-faucet repository (i.e., the miden-faucet-client crate) directly.

That's my understanding as well.

Moving this logic to the faucet repository would mean that the miden-faucet-client crate will directly use the miden-client crate as a library dependency to handle note consumption (e.g., syncing and consuming notes programmatically). For this to work correctly, we should implement a Client::from_system_user_config() function (as @igamigo posted in a previous message) in the miden-client crate that replicates the same logic the client CLI uses for determining: 1) the correct config TOML file and 2) the correct keystore directory.

It'd be great to be able to extract enough of the configuration details defined via clap so that they could be shared between both CLIs, but I don't have any sense of how easy or difficult that would be based on the current implementations of each.

100% agree though that the miden-client crate would need to provide the necessary high-level APIs needed to piggy-back off the current client configuration, without having to reimplement how the client configures itself.

Please let me know if everyone is fine with moving the functionality to the faucet repo and implementing this helper function in the client repository!

I think we have consensus on the overall approach here, and all that remains are some minor technical details related to client configuration that I don't see derailing things even if there is some objection there (and I doubt there is, but I'll defer to @igamigo on that). I'd say you have a green light to start building this out, and we can tackle any debate around technical details in the resulting PRs.

[Keinberger]

I think we have consensus on the overall approach here, and all that remains are some minor technical details related to client configuration that I don't see derailing things even if there is some objection there (and I doubt there is, but I'll defer to @igamigo on that). I'd say you have a green light to start building this out, and we can tackle any debate around technical details in the resulting PRs.

Sounds good, I already started building out the feature in the miden-client repository which can be found here: 0xMiden/miden-client#1642