-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: guide user to call a contract #306
base: main
Are you sure you want to change the base?
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.
Overall it LGTM. Some feedback:
┌ Pop CLI : Call a contract
│
◇ Where is your project located?
│ ./
│
◇ Paste the on-chain contract address:
│ 5DYs7UGBm2LuX4ryvyqfksozNAW5V47tPbGiVgnjYWCZ29bt
│
└ Unable to fetch contract metadata.
Error: Anyhow error: Cannot infer the root project id
It would be nice if “error” was not repeated. Something like this:
│
◇ Where is your project located?
│ ./
│
◇ Paste the on-chain contract address:
│ 5DYs7UGBm2LuX4ryvyqfksozNAW5V47tPbGiVgnjYWCZ29bt
│
└ Unable to fetch contract metadata.
Error: Cannot infer the root project id
Same with this:
◇ Do you want to execute the call? (Selecting 'No' will perform a dry run)
│ Yes
│
Error: Rpc error: RPC error: Error when opening the TCP socket: Connection refused (os error 61)
Caused by:
RPC error: Error when opening the TCP socket: Connection refused (os error 61)
Change to this:
◇ Do you want to execute the call? (Selecting 'No' will perform a dry run)
│ Yes
│
Error when opening the TCP socket: Connection refused (os error 61)
Caused by:
RPC error: Error when opening the TCP socket: Connection refused (os error 61)
◇ Where is your project located?
│ ./flipper
Autocomplete when typing the contract's location would be useful.
Looking up RPC URL's can be cumbersome. It would be nice to be able to select commonly used RPC URL's such as Pop Network or localhost.
So this:
◇ Where is your contract deployed?
│ wss://rpc1.paseo.popnetwork.xyz
Could be something like this:
◆ Where is your contract deployed?
│ ● Pop Network TestNet (wss://rpc1.paseo.popnetwork.xyz)
│ ○ Local on Port 9944 (ws://localhost:9944) ○ Custom (Specify)
Change this:
◆ Do you want to do another call?
│ ○ Yes / ● No
To something like this:
◆ Do you want to do another call?
│ ● No
│ ○ Yes, with the same contract
│ ○ Yes, with another contract on the same chain
If this change will not be incorporated then at least don’t clear the screen when I say “yes” to call another contract so that I can copy the address and chain URL. Regardless, the default for pop call contract
should be to never clear the screen when it is ran because I find it useful to have a “log” of calls that I have done either for copying info to use again or simply to remember the order of calls that I have done and with which parameters.
Change this:
◆ Select the message to call:
│ ● flip ( A message that can be called on instantiated contracts. This one flips the value of the stored `bool` from `true` to `false` and vice versa.)
│ ○ get
└
I would imagine these Rust Doc comments can easily be 4+ lines for a more sophisticated contract.
For example:
/// Use the new `call_v2` host function via the call builder to forward calls to
/// the other contract, initially calling `flip` and then `get` to return the
/// result.
///
/// This demonstrates how to set the new weight and storage limit parameters via
/// the call builder API.
Therefore I would have it be displayed underneath the function call making sure to preserve the same formatting that was presented in the Rust Doc comment e.g.:
◆ Select the message to call:
│ ● flip_and_get_invoke_v2_with_limits
|
│ Use the new `call_v2` host function via the call builder to forward calls to
│ the other contract, initially calling `flip` and then `get` to return the
│ result.
│
│ This demonstrates how to set the new weight and storage limit parameters via
│ the call builder API.
|
│ ○ get
└
◇ Signer calling the contract:
│ //Alice
For a custom account, I was not sure what to input so I pasted my account’s address:
◇ Signer calling the contract:
│ 16hkVKMehgVjTP7AsUvhU8rsYQhK9JDirPWWhpAwHaFTHRfc
Error: Failed to create keypair from URI: Cannot parse phrase: mnemonic has an invalid word count: 1. Word count must be 12, 15, 18, 21, or 24
It was not intuitive to me that it was expecting a mnemonic phrase like so:
│ car pocket team home penalty leg forget attend draft width blush lettuce
Thanks for the feedback @brunopgalvao Great point about placing the message docs below the message name I’ve fixed that:
Same for clearing the screen when prompting for another call. I liked your idea of reusing the contract information, but I reduced it to two options instead of the three you suggested. If want to call another contract, the user should start again:
Autocomplete for the path and a list of addresses for me are nice-to-have features we can add in the future, feel free to create issues if you think they are needed. As for the signer, I’m not sure how to improve that right now, but we’ll be working on a better process for signing transactions soon, so hopefully that will make things clearer. Another piece of feedback from @peterwht that has been implemented is showing the user the call that will be executed after all the values are prompted:
The option to skip the |
Some thoughts;
|
I like your idea to improving DevEx with less clicks. I’ve added the Regarding the --salt flag it works for me. Not it has to be an hex encoded value. I tried: |
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.
High level review only. I think addressing the suggested changes around the call contract command can greatly improve the UX and the simplify the implementation.
A more flexible experience provides a greater experience. With very little refactoring, I can partially specify args on the command line e.g. 'pop call contract --contract xx' and the call UI can be used if message is ommitted.
That means I can enter stuff into the command which I dont want to repeat over and over, and then allow the terminal history to bring it up and to get me back to the bits I want to specify.
I feel this should be addressed prior to this being merged as it improves the UX significantly.
Merged set_up_call_config and guide_user_to_call_contract into a single function. Also adds short symbols for 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.
Right now we assume that we are receiving a path to a contract project to load the metadata, and if not present build the contract to obtain the metadata.
We only need the metadata in order to call a contract, so it would be handy having both, an option for any user to point to their project and build the contract if necessary, or passing the location of the metadata directly to pop
. Maybe the user only has access to the metadata.
* fix: automatically add some or none to Option argument * test: refactor and tests * refactor: improve code and comments * fix: renaming and clean code * chore: option params not mandatory * fix: parse user inputs for Option arguments in constructor (#335) * fix: automatically add some or none to Option argument * fix: tests * refactor: process_function_args * test: update tests accordingly last changes * fix: issue with delimiter * test: fix unit test * refactor: renaming and fix comments * refactor: format types (#339) Shows the full type representation, making it easier to see the entry format of parameter values. * fix: logo doesn't show in README --------- Co-authored-by: Frank Bell <[email protected]> Co-authored-by: Alejandro Martinez Andres <[email protected]>
* chore: allow the user specify the metadata file to call a contract * test: unit test to parse metadata from a file * docs: fix docs * refactor: ensure_contract_built after user input path * fix: call contract when metadata file * fix: remove default_input in contract address * docs: rename metadata with artifact * fix: panic at has_contract_been_built * fix: clippy * refactor: keep ensure_contract_built as a CallContractCommand function * fix: ensure_contract_built * docs: improve comments * fix: feedback and include wasm file for testing
A note on this one, @AlexD10S . Right now if we execute |
Interesting. After some discussions the best approach seems to be prompting the user after build the contract if the contract has already been deployed. There's a valid case where the contract hasn't been built locally, but the user still wants to interact with an already deployed contract, and the local build is only a way to get the artifacts. |
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.
After all the improvements and addressing all the feedback the call contract flow looks pretty nice!
I believe there's one new function that could be divided in two. But aside from that all is looking great. Thanks!
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.
All is looking great! ❤️
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.
Whilst there are a lot of comments, most of them are minor. Once addressed I think we should be ready to merge.
@@ -1,5 +1,7 @@ | |||
// SPDX-License-Identifier: GPL-3.0 | |||
|
|||
#[cfg(feature = "contract")] |
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.
Why the need for this module, which has a single function, when there is already a contracts module?
/// # Arguments | ||
/// * `path` - An optional path to the project directory. If no path is provided, the current | ||
/// directory is used. | ||
pub fn has_contract_been_built(path: Option<&Path>) -> bool { |
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.
Move to existing common/contracts
module in the same crate. Can then remove this file completely.
/// directory is used. | ||
pub fn has_contract_been_built(path: Option<&Path>) -> bool { | ||
let project_path = path.unwrap_or_else(|| Path::new("./")); | ||
let manifest = match from_path(Some(project_path)) { |
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 be simplified with let Ok(manifest) = from_path(Some(project_path)) else { return false; };
/// The name of the contract message to call. | ||
#[clap(long, short)] | ||
message: String, | ||
message: Option<String>, | ||
/// The constructor arguments, encoded as strings. |
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.
/// The constructor arguments, encoded as strings. | |
/// The message arguments, encoded as strings. |
/// The constructor arguments, encoded as strings. | ||
#[clap(long, num_args = 0..)] | ||
#[clap(long, num_args = 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.
#[clap(long, num_args = 0..,)] | |
#[clap(short, long, num_args = 0..,)] |
.collect()) | ||
} | ||
|
||
/// Extracts the information of a smart contract constructor parsing the metadata file. |
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.
/// Extracts the information of a smart contract constructor parsing the metadata file. | |
/// Extracts the information of a smart contract constructor. |
metadata -> contract artifacts
/// Extracts the information of a smart contract constructor parsing the metadata file. | ||
/// | ||
/// # Arguments | ||
/// * `path` - Location path of the project. |
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.
/// * `path` - Location path of the project. | |
/// * `path` - Location path of the project or contract artifact. |
} | ||
|
||
/// Processes a list of argument values for a specified contract function, | ||
/// wrapping each value in `Some(...)` or replacing it with `None` if the argument is optional |
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.
/// wrapping each value in `Some(...)` or replacing it with `None` if the argument is optional | |
/// wrapping each value in `Some(...)` or replacing it with `None` if the argument is optional. |
/// wrapping each value in `Some(...)` or replacing it with `None` if the argument is optional | ||
/// | ||
/// # Arguments | ||
/// * `path` - Location path of the project. |
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.
/// * `path` - Location path of the project. | |
/// * `path` - Location path of the project or contract artifact. |
|
||
let value: BalanceVariant<<DefaultEnvironment as Environment>::Balance> = | ||
parse_balance(&call_opts.value)?; | ||
|
||
let contract: <DefaultConfig as Config>::AccountId = parse_account(&call_opts.contract)?; | ||
// Process the argument values input by the user. |
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.
// Process the argument values input by the user. | |
// Process the provided argument values. |
From the perspective of a library crate, there is no guarantee that it has been input by a user. It could be read from a file as an example.
PR that parses the contract metadata and guides the user in interacting with the smart contract
Apologies for the large PR; it grew bigger as we needed to test user interactions in the
pop-cli
crate.This required adding more code for testing purposes, including changes to the
CLI module
and the introduction of a newinit_tests
file which contains reusable methods for various tests.Ideally, these changes should have been introduced in #315, but they were included here due to the necessity of testing the current implementation.
Another note, for testing purposes it has introduced a
testing.json
file. This file is the metadata of a modifiedflipper
contract, which includes an extra message, to effectively test argument parsing and payable functions.