-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Implement 'balance' command to fetch wallet balance #152
base: main
Are you sure you want to change the base?
Conversation
src/client/mod.rs
Outdated
None => { | ||
if wallet_info.wallet_currency == WalletCurrency::USD { | ||
wallet_balances += &format!( | ||
"USD wallet{}: {} cents\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we exporting the value always as a string? would be good to have the option to expose it as a json as well (can be done in another PR later in the project)
a key goal behind galoy-cli is to enable scripting. which is why considering json output as much as friendly output is important
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the time being, the main focus of this command is to output the balance in the CLI, hence the string format. However, as you've suggested, as we further develop the cli, the necessity for the function to return a number or a JSON will definitely arise as this function will be used in other functions or tests as well.
it would be relevant to consider been able to pass an array of WalletId as well as an arguments. right now we only have 2 wallets per account, so this will works, but this assumption of only having 2 wallets will expend eventually for business account. |
@nicolasburtey I have addressed this in commit 4a65bd3. The updated command now supports an array of wallet IDs provided as comma-separated values in the |
src/client/mod.rs
Outdated
@@ -97,6 +100,61 @@ impl GaloyClient { | |||
Ok(me) | |||
} | |||
|
|||
pub fn fetch_balance( | |||
&self, | |||
wallet: Option<Wallet>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the wallet
parameter there?
shouldn't it rather be walletType
?
and where is it used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the option to specify --btc
or --usd
along with the wallet_ids. So here wallet
is used when one of these options is given as input.
Renamed it to wallet_type
based on your feedback. 626873f
64ab5f2
to
330afe0
Compare
This Pull Request introduces a new command,
balance
, to the CLI. Thebalance
command is used to fetch the user's balance from their wallet.Users can fetch their balance in four different ways:
balance --usd
: Fetches the balance in the USD wallet.balance --btc
: Fetches the balance in the BTC wallet.balance
orbalance --usd --btc
: Fetches the balance from both the wallets and specifies the default one.In the process of making these changes, the hardcoded
Wallet
type inmain.rs
has been moved to a new file,types.rs
, which is now being exported fromlib.rs
. This centralizes the handling of types and makes the code more scalable for future expansions.Issue: #145