Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: forc-deploy asks for password before checking if the wallet exists #6704

Merged
merged 2 commits into from
Nov 9, 2024

Conversation

kayagokalp
Copy link
Member

Description

closes #6703 and unblocks #6680.

Forc-deploy was asking for a password without checking if a wallet exists or not and the check for existence was happening after the password prompt. This meant we were asking for a password even when the wallet does not exists.

Below examples are taken in a system where the wallet does not exists at the default path (a fresh system):

Before

...
  Confirming transactions [deploy final-contract]
             Network: https://testnet.fuel.network
             Wallet: /Users/kayagokalp/.fuel/wallets/.wallet
✔ Wallet password · ****
? Could not find a wallet at "/Users/kayagokalp/.fuel/wallets/.wallet", would you like to create a new one? [y/N]:  (y/n) ›

After

...
  Confirming transactions [deploy final-contract]
             Network: https://testnet.fuel.network
? Could not find a wallet at "/Users/kayagokalp/.fuel/wallets/.wallet", would you like to create a new one? [y/N]:  (y/n) ›

@kayagokalp kayagokalp added the forc-deploy Everything to do with forc-deploy label Nov 8, 2024
@kayagokalp kayagokalp self-assigned this Nov 8, 2024
@kayagokalp kayagokalp marked this pull request as ready for review November 8, 2024 00:31
@kayagokalp kayagokalp requested a review from a team as a code owner November 8, 2024 00:31
@kayagokalp kayagokalp added the bug Something isn't working label Nov 8, 2024
zees-dev
zees-dev previously approved these changes Nov 8, 2024
Copy link
Contributor

@zees-dev zees-dev left a comment

Choose a reason for hiding this comment

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

Lgtm 👍

sdankel
sdankel previously approved these changes Nov 8, 2024
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.

lgtm, thanks!

@sdankel
Copy link
Member

sdankel commented Nov 8, 2024

Ah, there's a failing test.

@kayagokalp kayagokalp dismissed stale reviews from sdankel and zees-dev via 29e4562 November 8, 2024 07:27
@kayagokalp
Copy link
Member Author

Updated and it should be good now :)

The failing test assumed it was testing wrong password but it was not. Due to the same bug that is being solved with this PR, cli was asking a password for a wallet that does not exists. Updated the test, as the relevant section in the code also states we need to be able to override the default path for tests to test the happy path as well. I'll try to look into it next week 👌

@kayagokalp
Copy link
Member Author

Thinking about this I am curious what happens if you run cargo -t locally after this change if you a have a wallet locally will the test fail 🤔 We should probably think about overriding the default path for tests so that the tests ensure reproducability

@kayagokalp kayagokalp merged commit efa4aab into master Nov 9, 2024
38 checks passed
@kayagokalp kayagokalp deleted the kayagokalp/6703 branch November 9, 2024 02:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working forc-deploy Everything to do with forc-deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

forc-deploy asks password for non-existing wallet
3 participants