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

dexc: change application password feature. #978

Merged
merged 1 commit into from Mar 27, 2021

Conversation

amass01
Copy link
Member

@amass01 amass01 commented Feb 16, 2021

Closes #678

client/core/core.go Outdated Show resolved Hide resolved
@amass01 amass01 force-pushed the changepass branch 2 times, most recently from 2df9ec2 to 2a725e2 Compare March 5, 2021 00:13
client/core/core.go Outdated Show resolved Hide resolved
@amass01 amass01 changed the title [wip] dexc: change application password feature. dexc: change application password feature. Mar 5, 2021
@amass01 amass01 marked this pull request as ready for review March 5, 2021 16:11
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/types.go Outdated Show resolved Hide resolved
client/db/bolt/db.go Outdated Show resolved Hide resolved
@chappjc chappjc added this to the 0.2 milestone Mar 6, 2021
@amass01
Copy link
Member Author

amass01 commented Mar 8, 2021

@chappjc Updated

@amass01 amass01 force-pushed the changepass branch 3 times, most recently from 4959db8 to 9d326c8 Compare March 8, 2021 18:15
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

comment message of 9d326c8 maybe should change mutuable -> mutable

The behavior when you enter something wrong differs for these fields:
notgoaway

If you enter the wrong current app pass, the current and new are cleared, but the confirm new is not. If new and confirm are different, it errors but none are cleared. Maybe all should be cleared for any mistake or success?

Unrelated to this pr:
I had the simnet test fail once:

--- FAIL: TestResendPendingRequests (39.07s)
    trade_simnet_test.go:823: [client 2] dcr balance change not in expected range 19.00000000 - 20.00000000, got 9.99993000
FAIL
exit status 1
FAIL    decred.org/dcrdex/client/core   500.617s

And I see this as an error in client logs, but should maybe be a warning?

[ERR] WEB: max order estimation error: dcr wallet MaxOrder error: error getting network fee estimate: -32603: no bucket with the minimum required success percentage found

client/core/types.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
return err
}
// Update xcWallets' encrypted passwords.
err = c.updateWalletsEncPW(oldCrypter, newCrypter)
Copy link
Member

Choose a reason for hiding this comment

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

An error here or below would be fairly devastating, since the password has already been changed, because presumably the wallet's encPW updates are incomplete.

Ideally, this entire operation would be a db.DB method performed using a single bbolt.Tx, but since we are using the Store method for the key params, that gets a little sloppy. This is a consequence of bad design on my part. The general purpose Store and Get methods are cheap and I'm already getting rid of them. I'm okay with this in the meantime. I plan to follow up quickly.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/webserver/site/src/js/register.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/register.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/settings.js Outdated Show resolved Hide resolved
client/webserver/site/src/js/settings.js Outdated Show resolved Hide resolved
client/webserver/site/src/html/forms.tmpl Show resolved Hide resolved
@amass01 amass01 force-pushed the changepass branch 2 times, most recently from 3aabdc6 to f739e48 Compare March 11, 2021 10:29
client/core/core.go Outdated Show resolved Hide resolved
client/core/wallet.go Outdated Show resolved Hide resolved
client/core/wallet.go Outdated Show resolved Hide resolved
client/core/trade_simnet_test.go Outdated Show resolved Hide resolved
client/core/wallet.go Outdated Show resolved Hide resolved
client/core/wallet.go Outdated Show resolved Hide resolved
@amass01 amass01 force-pushed the changepass branch 2 times, most recently from 4f823e1 to 326eff0 Compare March 17, 2021 13:48
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

I had forgotten a pending review, sorry. Just a couple tiny nits, but it's working for me.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Show resolved Hide resolved
@amass01
Copy link
Member Author

amass01 commented Mar 20, 2021

@chappjc updated

@amass01 amass01 requested a review from buck54321 March 22, 2021 19:58
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Tests well.

client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core_test.go Outdated Show resolved Hide resolved
client/core/types.go Outdated Show resolved Hide resolved
client/webserver/site/src/js/settings.js Outdated Show resolved Hide resolved
@amass01
Copy link
Member Author

amass01 commented Mar 23, 2021

@buck54321 updated mate

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

One last thing, but looks great.

client/webserver/site/src/html/settings.tmpl Outdated Show resolved Hide resolved
@amass01
Copy link
Member Author

amass01 commented Mar 27, 2021

Needs a rebase after #985.

@chappjc
Copy link
Member

chappjc commented Mar 27, 2021

@amassarwi I just rebased it. Sorry didn't see your comment

@amass01
Copy link
Member Author

amass01 commented Mar 27, 2021

@chappjc all good, thank you

@chappjc chappjc merged commit 8d7163c into decred:master Mar 27, 2021
@amass01 amass01 deleted the changepass branch March 27, 2021 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

app: reset password and change password features
4 participants