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: integrate forc-wallet and forc-deploy #4752

Merged
merged 7 commits into from
Jul 6, 2023
Merged

Conversation

kayagokalp
Copy link
Member

Description

closes #4743.

This PR integrates forc-wallet and forc-deploy together so that forc-deploy can offer to sign your transactions without running a second terminal window. Below there are some inputs and outputs after this change.

forc-deploy --random-salt
Contract id: 0x77e0c380dff45759d1a16b6add83d0fd38e8d86ddd0c96cd5117c82b6d958edb

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

At this point if the user enters wrong password:

Contract id: 0x77e0c380dff45759d1a16b6add83d0fd38e8d86ddd0c96cd5117c82b6d958edb

Please provide the password of your encrypted wallet vault at "/Users/kayagokalp/.fuel/wallets/.wallet":
Error: Failed to access forc-wallet vault. Please check your password

If the user enters correct password:

Contract id: 0x77e0c380dff45759d1a16b6add83d0fd38e8d86ddd0c96cd5117c82b6d958edb

Please provide the password of your encrypted wallet vault at "/Users/kayagokalp/.fuel/wallets/.wallet":
Do you accept to sign this transaction with fuel15f7mn9vue4eks9znjqf5tg6ylv8vkfh9xcdh3v6z9s7yagewgwasaxpxem [y/N]:

If the user enters y or Y and hits enter:

Contract id: 0x77e0c380dff45759d1a16b6add83d0fd38e8d86ddd0c96cd5117c82b6d958edb

Please provide the password of your encrypted wallet vault at "/Users/kayagokalp/.fuel/wallets/.wallet":
Do you accept to sign this transaction with fuel15f7mn9vue4eks9znjqf5tg6ylv8vkfh9xcdh3v6z9s7yagewgwasaxpxem [y/N]:y
contract 98f158f121c10bdfea5ffbdda904d3e7b61c7f9d80ae2beff75b5d04f0f3547b deployed in block 0x1dd25813f6a82a69347079feb265eeeb37e89f992cea65ad1373b378b80c1ecb

If the user enters anything else:

Contract id: 0x77e0c380dff45759d1a16b6add83d0fd38e8d86ddd0c96cd5117c82b6d958edb

Please provide the password of your encrypted wallet vault at "/Users/kayagokalp/.fuel/wallets/.wallet":
Do you accept to sign this transaction with fuel15f7mn9vue4eks9znjqf5tg6ylv8vkfh9xcdh3v6z9s7yagewgwasaxpxem [y/N]:N
Error: User refused to sign

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 forc-deploy Everything to do with forc-deploy labels Jul 5, 2023
@kayagokalp kayagokalp self-assigned this Jul 5, 2023
@kayagokalp kayagokalp requested a review from a team July 5, 2023 13:17
@kayagokalp kayagokalp marked this pull request as ready for review July 5, 2023 13:17
sdankel
sdankel previously approved these changes Jul 5, 2023
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.

utACK

forc-plugins/forc-client/src/util/tx.rs Show resolved Hide resolved
@kayagokalp kayagokalp requested a review from JoshuaBatty July 5, 2023 21:01
@kayagokalp
Copy link
Member Author

Also will keep this waiting, won't merge right away because I need a release for fuel.nix stuff with #4750 merged. I want to omit releasing a version with this (before updating the UX). So my current plan looks like:

  1. Merge refactor: move sysinfo behind a feature flag and only enable it for non-macos builds #4750
  2. Cut a sway repo release
  3. Merge this one
  4. Merge UX improvements
  5. Cut another release

So that if anything comes up (very bad bugs with this etc) we do not block fuel.nix resolutions any longer, and have a release that both works for fuel.nix and without this changes.

JoshuaBatty
JoshuaBatty previously approved these changes Jul 6, 2023
@JoshuaBatty
Copy link
Member

Instead of typing forc-deploy --random-salt can --random-salt just be on by default? Perhaps this is already the case was just confused that it was in the description.

@kayagokalp kayagokalp dismissed stale reviews from sdankel and JoshuaBatty via c2e5dca July 6, 2023 11:31
@kayagokalp kayagokalp requested review from sdankel, JoshuaBatty and a team July 6, 2023 11:31
@JoshuaBatty JoshuaBatty requested a review from a team July 6, 2023 11:39
@kayagokalp kayagokalp requested a review from a team July 6, 2023 11:41
@kayagokalp kayagokalp enabled auto-merge (squash) July 6, 2023 11:59
Copy link
Contributor

@vaivaswatha vaivaswatha left a comment

Choose a reason for hiding this comment

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

Approving just for admin reasons (two reviews needed) as this was already approved before and just had a merge conflict fixed.

@kayagokalp kayagokalp merged commit 87b6228 into master Jul 6, 2023
@kayagokalp kayagokalp deleted the kayagokalp/4743 branch July 6, 2023 12:06
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
Development

Successfully merging this pull request may close these issues.

forc-deploy should be integrated with forc-wallet
4 participants