refactor(wallet)!: rename method to last_unused_address
#1680
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oleonardolima
force-pushed
the
refactor/next-unused-address-rename-to-last-unused-address
branch
from
168fbbe
to
83f86cd
Compare
As per both out-of-band discussions, and the latest discussion at weekly development call, the semantics of this method can be misleading, as it's not guaranteed you get a next one on every call, that'd only happen if the address has been used and the application synced with latest blockchain state. Renaming it to `last_unused_address` gives it a better semantics, as you'll always get the last derived address which still unused. As per usage where the user needs always a new derived address on every call, you should still use the `reveal_next_address`, nothing changed.
oleonardolima
force-pushed
the
refactor/next-unused-address-rename-to-last-unused-address
branch
from
83f86cd
to
07b85ca
Compare
notmandatory
previously approved these changes
Nov 19, 2024
|
Okay thinking about it, this method actually returns the earliest unused... |
|
lets make this a doc only change |
notmandatory
added
documentation
|
As per last call discussion, I'll repurpose this PR to keep |
notmandatory
dismissed
their stale review
Will re-review after new direction changes pushed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
As per both out-of-band discussions, and the latest discussion at weekly development call, the semantics of this method can be misleading, as it's not guaranteed you get a next one on every call, that'd only happen if the address has been used and the application synced with latest blockchain state.
Renaming it to
last_unused_addressgives it a better semantics, as you'll always get the last derived address which still unused. As per usage where the user needs always a new derived address on every call, you should still use thereveal_next_address, nothing changed.Notes to the reviewers
I'm open for discussions/suggestions on naming alternatives and better semantics. I think
last_unused_addressis pretty good, but maybe you can come up with another better one.Changelog notice
Wallet::next_unused_addressmethod toWallet::last_unused_address.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
Bugfixes: