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

Forc-deploy import wallet support if non-existent wallet #6680

Merged
merged 7 commits into from
Nov 9, 2024

Conversation

zees-dev
Copy link
Contributor

Description

@zees-dev zees-dev added the forc-deploy Everything to do with forc-deploy label Oct 31, 2024
@zees-dev zees-dev requested review from a team as code owners October 31, 2024 05:06
@zees-dev zees-dev added the enhancement New feature or request label Oct 31, 2024
alfiedotwtf
alfiedotwtf previously approved these changes Nov 1, 2024
sdankel
sdankel previously requested changes Nov 4, 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.

The prompt about creating/importing a wallet should come before asking for the wallet password. Otherwise, once the tests are passing, LGTM 😃

image

@zees-dev
Copy link
Contributor Author

zees-dev commented Nov 4, 2024

The prompt about creating/importing a wallet should come before asking for the wallet password. Otherwise, once the tests are passing, LGTM 😃

The current behaviour performs the password check which is much more upstream in the code; changing that would imply a re-factor which may have knock-on effects.
I think this would be worthwhile addressing as a seperate issue.
If we have agreement; I can create an issue for that.

@kayagokalp
Copy link
Member

kayagokalp commented Nov 4, 2024

The prompt about creating/importing a wallet should come before asking for the wallet password. Otherwise, once the tests are passing, LGTM 😃

The current behaviour performs the password check which is much more upstream in the code; changing that would imply a re-factor which may have knock-on effects. I think this would be worthwhile addressing as a seperate issue. If we have agreement; I can create an issue for that.

I think we should discuss the necessary refactor to do this, because it feels a little confusing as it is. If i don't have a wallet it is highly likely that I don't have necessary context to understand what password I need to enter once that is prompted. So it might be worth doing the refactor before merging this.

If you can come up with some kind of a description of what you need to be changed exactly I am more than happy to help out! I want to clean forc deploy paths for some time now even have a tracking issue for it. #6259

@zees-dev
Copy link
Contributor Author

zees-dev commented Nov 6, 2024

The prompt about creating/importing a wallet should come before asking for the wallet password. Otherwise, once the tests are passing, LGTM 😃

The current behaviour performs the password check which is much more upstream in the code; changing that would imply a re-factor which may have knock-on effects. I think this would be worthwhile addressing as a seperate issue. If we have agreement; I can create an issue for that.

I think we should discuss the necessary refactor to do this, because it feels a little confusing as it is. If i don't have a wallet it is highly likely that I don't have necessary context to understand what password I need to enter once that is prompted. So it might be worth doing the refactor before merging this.

If you can come up with some kind of a description of what you need to be changed exactly I am more than happy to help out! I want to clean forc deploy paths for some time now even have a tracking issue for it. #6259

I believe this should be done in another PR as the scope could become much larger.
In this PR we just want to give option for user to import seed phrase; without changing/refactoring the upstream flow.

@kayagokalp
Copy link
Member

kayagokalp commented Nov 7, 2024

The prompt about creating/importing a wallet should come before asking for the wallet password. Otherwise, once the tests are passing, LGTM 😃

The current behaviour performs the password check which is much more upstream in the code; changing that would imply a re-factor which may have knock-on effects. I think this would be worthwhile addressing as a seperate issue. If we have agreement; I can create an issue for that.

I think we should discuss the necessary refactor to do this, because it feels a little confusing as it is. If i don't have a wallet it is highly likely that I don't have necessary context to understand what password I need to enter once that is prompted. So it might be worth doing the refactor before merging this.
If you can come up with some kind of a description of what you need to be changed exactly I am more than happy to help out! I want to clean forc deploy paths for some time now even have a tracking issue for it. #6259

I believe this should be done in another PR as the scope could become much larger. In this PR we just want to give option for user to import seed phrase; without changing/refactoring the upstream flow.

I don't think we should merge this as it is because as it is this will be confusing, my suggestion would be to keep this ready to go and first fix the upstream and after that using the already fixed upstream let's merge this PR.

I am curious about what needs to be changed to fix this issue and if i understood correctly those needs to be fixed in forc-wallet, right?

Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

It turns out this situation is present for creating a new wallet as well. I opened #6704, once merged it will also fix the problem we are having here. If we can merge that and rebase this one wallet password won't be prompted.

@zees-dev zees-dev dismissed sdankel’s stale review November 9, 2024 20:32

Changes have been made

@zees-dev zees-dev merged commit f933f55 into master Nov 9, 2024
38 checks passed
@zees-dev zees-dev deleted the fix/issue-139 branch November 9, 2024 20:32
# for free to join this conversation on GitHub. Already have an account? # 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.

Prompt user to enter seed phrase whenever we prompt them to create a new wallet
4 participants