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

feat: show a list of accounts and their balances before selecting an account #4771

Merged
merged 11 commits into from
Aug 8, 2023

Conversation

kayagokalp
Copy link
Member

@kayagokalp kayagokalp commented Jul 7, 2023

Description

closes #4757.
closes #4756.

This PR adds balance query capabilities to forc-deploy so that users can select the desired account to use before signing their transaction. Current output looks like:

Please provide the password of your encrypted wallet vault at "/Users/kayagokalp/.fuel/wallets/.wallet":

Account 0 -- fuel1x6pc0g9gwp2c2hzhejlw7l4guqd99732zt2wd68j2ukrdu59kv9qqz9h3k:
  Asset ID                                                           Amount
  0000000000000000000000000000000000000000000000000000000000000000 4294967295

Please provide the index of account to use for signing:0
Do you accept to sign this transaction with fuel1x6pc0g9gwp2c2hzhejlw7l4guqd99732zt2wd68j2ukrdu59kv9qqz9h3k? [y/N]: y


Contract deploy_test_integration Deployed!

Network: http://127.0.0.1:4000
Contract ID: 0x73ea64a310b6fc2c9e6674a49f5515f01950d97cc845cb4149ececea6cbf4060
Deployed in block 0x45ee78fed1083c7734246cb93258dcdc865fc9bc13667b89088aeb43037

Also now forc-deploy throws an error for empty accounts:

Please provide the password of your encrypted wallet vault at "/Users/kayagokalp/.fuel/wallets/.wallet":
Error: Your wallet does not have any funds to pay for the deployment transaction.
If you are deploying to a testnet consider using the faucet.
If you are deploying to a local node, consider providing a chainConfig which funds your account.

Once we integrate a nice TUI selection library the output could be more interactive with up and down keys etc.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@kayagokalp kayagokalp added enhancement New feature or request blocked forc-deploy Everything to do with forc-deploy labels Jul 7, 2023
@kayagokalp kayagokalp self-assigned this Jul 7, 2023
@JoshuaBatty JoshuaBatty mentioned this pull request Jul 7, 2023
7 tasks
@kayagokalp
Copy link
Member Author

Blocked by #4718

kayagokalp added a commit that referenced this pull request Jul 20, 2023
## Description
Unused dependency check is failing as nightly version that we use for
that step is too old for fuels-program v0.44 dependency which is being
added in a separate PR. (failing PR: #4771)
@kayagokalp
Copy link
Member Author

I am not sure what is causing it but mdbook documentor starting throwing issues while compiling with these changes and changes here is totally independent from each other but to make the compiler happy i am fixing it with this PR as well.

@kayagokalp kayagokalp requested a review from a team July 26, 2023 09:06
@kayagokalp kayagokalp marked this pull request as ready for review July 26, 2023 09:06
@kayagokalp kayagokalp enabled auto-merge (squash) July 26, 2023 09:06
@kayagokalp kayagokalp disabled auto-merge July 26, 2023 09:06
@kayagokalp kayagokalp enabled auto-merge (squash) July 26, 2023 09:07
JoshuaBatty
JoshuaBatty previously approved these changes Jul 28, 2023
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

Looks good, just left a comment to open an issue for the todo if one hasn't been opened already.

@JoshuaBatty JoshuaBatty requested a review from a team July 28, 2023 01:53
@kayagokalp
Copy link
Member Author

Looks good, just left a comment to open an issue for the todo if one hasn't been opened already.

Forgot to reply! sorry for that. These two are already open:

  1. forc-deploy should link to faucet for beta-4 once the link is ready #4862
  2. forc-deploy should automatically derive the first account after creating a new wallet #4861

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

Nice work! A couple suggestions/questions:

  1. It would be nice to align the amount underneath the "Amount" heading, so the first digit of the amount is directly underneath "A"
  2. Have you tested it with multiple accounts? I'm curious what the TUI looks like in that case.
  3. What happens in the case where there are no accounts? Does it show the same message as total_balance == 0? We should probably show a different error message for that case.

forc-plugins/forc-client/src/util/tx.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/util/tx.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/util/tx.rs Outdated Show resolved Hide resolved
forc-plugins/forc-client/src/util/tx.rs Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
Co-authored-by: Sophie Dankel <47993817+sdankel@users.noreply.github.com>
@kayagokalp
Copy link
Member Author

kayagokalp commented Aug 2, 2023

What happens in the case where there are no accounts? Does it show the same message as total_balance == 0? We should probably show a different error message for that case.

If there are no wallet to be used a different error is returned

let question = format!("Could not find a wallet at {wallet_path:?}, would you like to create a new one? [y/N]: ");

And there will always be an account. If there is a wallet. Once i fix #4861.

Have you tested it with multiple accounts? I'm curious what the TUI looks like in that case.
This is the same TUI offered by forc-wallet, FuelLabs/forc-wallet#95 you can see the example output with multiple accounts.

It would be nice to align the amount underneath the "Amount" heading, so the first digit of the amount is directly underneath "A"

Again this is not something I can do here with this PR as this is how forc-wallet prints it. If you feel like we should change let's change it there.

@kayagokalp kayagokalp requested a review from a team August 3, 2023 11:40
@JoshuaBatty
Copy link
Member

Again this is not something I can do here with this PR as this is how forc-wallet prints it. If you feel like we should change let's change it there.

It would be cleaner to have the alignment as Sophie mentioned. Would you mind opening an issue for this in forc-wallet so we can add this in a future PR?

@JoshuaBatty JoshuaBatty requested a review from a team August 4, 2023 00:41
@kayagokalp
Copy link
Member Author

Opened this one 🙌 will tackle it right away once we clear urgent stuff cc @sdankel @JoshuaBatty

@kayagokalp kayagokalp merged commit d6035a9 into master Aug 8, 2023
@kayagokalp kayagokalp deleted the kayagokalp/4757 branch August 8, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request forc-deploy Everything to do with forc-deploy
Projects
None yet
3 participants