-
Notifications
You must be signed in to change notification settings - Fork 0
feat: create account from Package #25
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
base: next
Are you sure you want to change the base?
Conversation
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
/// Optional list of files specifying additional component template files to add to the | ||
/// account. | ||
/// List of files specifying component template files for the account. At | ||
/// lease one component template is required. |
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.
/// lease one component template is required. | |
/// least one component template is required. |
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.
Thanks! Changed here: b285982
// If a user passes in a file with the `.masp` file extension, then we | ||
// leave the passed in path as is; since it probably is a full path. | ||
// This is the case with cargo-miden, which displays the full path to | ||
// stdout after compilation finishes. | ||
let path = if path.extension().is_none() { | ||
let path = path.with_extension(MIDEN_PACKAGE_EXTENSION); | ||
packages_dir.join(&path) | ||
} else { | ||
path.clone() | ||
}; |
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.
But we're not checking that this file's extension is actually .masp, right? Only that it exists.
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.
Great point! I'll add a check.
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.
Check added here: eb6884a
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
/// Optional list of files specifying additional component template files to add to the | ||
/// account. | ||
/// List of files specifying component template files for the account. At | ||
/// least one component template is required. |
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.
Sidenote: I believe this list is not optional since at least one element has to be passed in, source: https://github.com/lambdaclass/miden-client/pull/25/files#diff-95eb60497fe9b9ad958ac1c26595181b35ee51f5e192ebe32ac9689cecb768c2L254-R360.AFAIK, this is only optional when building a wallet.
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
Signed-off-by: Tomas Fabrizio Orsi <[email protected]>
27b99dd
to
90c41b3
Compare
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.
LGTM
This PR adds makes it so that the CLI can now extract a component template from a passed in package, via the
-p, --packages
flags.Additionally, this TODO comment was addressed, now the I/O error display more context.
Here are snippets showcasing the new error messages: