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

refactor!: remove outdated wallet_ffi balance methods #3528

Conversation

StriderDM
Copy link
Contributor

@StriderDM StriderDM commented Nov 3, 2021

Description
Removed outdated balance methods in wallet_ffi
Updated wallet.h
Updated cucumber tests

Motivation and Context
Removal of outdated methods

Merge #3547 first.

How Has This Been Tested?
nvm use 12.22.6 && node_modules/.bin/cucumber-js --profile "ci" --tags "not @long-running and not @broken and @wallet-ffi"

Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

So I see that the various Balance helper's exist but that the only way to get a balance after these methods are removed is via the callbacks. I think there still needs to be an explicit method to request the balance. So please include a wallet_get_balance(...) method in this PR that returns the current TariBalance.

The title of this breaking PR should have a ! in it to indicate it is a breaking change i.e. refactor!: remove outdated wallet_ffi balance methods

base_layer/wallet_ffi/Cargo.toml Outdated Show resolved Hide resolved
@StriderDM
Copy link
Contributor Author

@philipr-za Your last comment with regards to wallet_get_balance(...) is a change of scope for this item. I am inclined to agree and will make the necessary changes to this PR.

@StriderDM StriderDM force-pushed the wallet_ffi_remove_outdated_balance_methods branch 2 times, most recently from 41fd05f to 19e8fa8 Compare November 8, 2021 13:38
@StriderDM StriderDM changed the title refactor: remove outdated wallet_ffi balance methods refactor!: remove outdated wallet_ffi balance methods Nov 8, 2021
@StriderDM StriderDM force-pushed the wallet_ffi_remove_outdated_balance_methods branch 2 times, most recently from 05139ea to b57d586 Compare November 8, 2021 13:43
@StriderDM StriderDM force-pushed the wallet_ffi_remove_outdated_balance_methods branch 2 times, most recently from 606cd5b to 7af25fb Compare November 8, 2021 21:31
philipr-za
philipr-za previously approved these changes Nov 9, 2021
hansieodendaal
hansieodendaal previously approved these changes Nov 9, 2021
Copy link
Contributor

@hansieodendaal hansieodendaal left a comment

Choose a reason for hiding this comment

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

LGTM

Note: See suggested name changes for future consideration.

@@ -321,6 +337,22 @@ class Wallet {
return result;
}

getBalance() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method would be more aptly named getBalanceFromCallback() to minimize confusion

return this.balance;
}

pollBalance() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method would be more aptly named getBalanceFromWallet() to minimize confusion (see related comment)

Comment on lines +77 to +78
pollBalance() {
return this.wallet.pollBalance();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these methods would be more aptly named getBalanceFromWallet() to minimize confusion (see related comment)

Removed outdated balance methods
Updated wallet.h
Updated cucumber tests

Review comments

cargo-fmt
@StriderDM StriderDM force-pushed the wallet_ffi_remove_outdated_balance_methods branch from a50bd5b to 35efa34 Compare November 9, 2021 09:12
@aviator-app aviator-app bot merged commit 413757b into tari-project:development Nov 9, 2021
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.

None yet

3 participants