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

feat(wallets.py): add wallet history command #1638

Merged

Conversation

saqib-codes-11
Copy link
Contributor

Description :-

This PR adds the following new command to fetch all the transfers associated with the provided "Wallet Name".

Command - btcli wallet history --wallet.name xyz

Screenshot -

Screenshot 2024-01-02 at 09 58 52

Copy link
Contributor

@ifrit98 ifrit98 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Looks good overall, left you some comments with a request for updating the while loop.

bittensor/commands/wallets.py Show resolved Hide resolved
transfers = []

# Make the request with the provided query and variables until all transfers are fetched
while True:
Copy link
Contributor

@ifrit98 ifrit98 Jan 2, 2024

Choose a reason for hiding this comment

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

Generally weary of unbounded while True statements in python. This technically should terminate on the desired condition, but may want to explicitly put the exit condition in this line and/or specify some other conditions for exiting. For example, if there is significant txn volume, we may want to limit the output to n txns.

For example, adding a max_txn_history that we exit upon reaching:

max_txn_history = 1000 # this should be set in the config.
page_info = {"page_info": {"hasNextPage": True}} # for initial loop entry, will be filled by `transfer_data`
while len(transfers) < max_txn_history and page_info.get("hasNextPage") == True:
    ...

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it will be a better approach. Will be fixing this in the next commit.

Copy link

Choose a reason for hiding this comment

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

You not need while. Change 10 to 1000 or more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alex0500 removed while and set max_txn to 1000.

@ifrit98
Copy link
Contributor

ifrit98 commented Jan 2, 2024

Also, in the future, would you mind branching off from staging? This is the preferred workflow outlined in the contrib docs for development.

Thanks!

@ifrit98 ifrit98 changed the base branch from master to staging January 2, 2024 19:52
@ifrit98
Copy link
Contributor

ifrit98 commented Jan 2, 2024

Any new feature should include proper tests for them, including new btcli functions. Make sure to add this as well in tests/integration_tests/test_cli.py and/or tests/integration_tests/test_cli_no_network.py where appropriate.

Thanks!

@saqib-codes-11
Copy link
Contributor Author

Any new feature should include proper tests for them, including new btcli functions. Make sure to add this as well in tests/integration_tests/test_cli.py and/or tests/integration_tests/test_cli_no_network.py where appropriate.

Thanks!

Added

@ifrit98 ifrit98 self-requested a review January 3, 2024 19:50
Copy link
Contributor

@ifrit98 ifrit98 left a comment

Choose a reason for hiding this comment

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

Thanks for the updated tests and testing out the query. Looks good!

Two small nits for you and we should be good to merge:
(1) Don't unlock coldkey to get ss58_address
(2) Display values in tao instead of rao (and mention this in the column, e.g. Amount (tao)

Suggestion for a nice to have, but is not necessary IMO:
(3) May be nice to add a column for Time, translating the block number into datetimes. What do you think?

... |  Block No  | Time
... |   2044685  | Nov 1, 2023 12:21:00 
...

def run(cli):
r"""Check the transfer history of the provided wallet."""
wallet = bittensor.wallet(config=cli.config)
wallet_address = wallet.coldkey.ss58_address
Copy link
Contributor

Choose a reason for hiding this comment

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

It is somewhat cumbersome to enter the password each time to simply get the coldkey address, which is not exactly private data.

Suggestion:

wallet_address = wallet.get_coldkeypub().ss58_address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done.

item["id"],
item["from"],
item["to"],
item["amount"],
Copy link
Contributor

Choose a reason for hiding this comment

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

The current amount displayed is in rao, and displaying in tao may be more user-friendly. It's a simple divide by 1e9

Suggestion:

int(item["amount"]) / 1e9

May want to do some error handling if the value cannot be converted to an int.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also done. Tested it for the below values,
2
2.9
"2"
"2.989767"

Any invalid integer will throw an error, in that case, I am using it as it as. Any other suggestions on the same?

@alex0500
Copy link

alex0500 commented Jan 3, 2024

Where is time of transfers?

@saqib-codes-11
Copy link
Contributor Author

saqib-codes-11 commented Jan 4, 2024

Thanks for the updated tests and testing out the query. Looks good!

Two small nits for you and we should be good to merge: (1) Don't unlock coldkey to get ss58_address (2) Display values in tao instead of rao (and mention this in the column, e.g. Amount (tao)

Suggestion for a nice to have, but is not necessary IMO: (3) May be nice to add a column for Time, translating the block number into datetimes. What do you think?

... |  Block No  | Time
... |   2044685  | Nov 1, 2023 12:21:00 
...

I did complete the first two changes but I was not able to get the correct time using the block number,

I tried the below formula to calculate it but was getting a 1-day difference.

from datetime import datetime, timedelta
time_for_target_block = timedelta(seconds=(target_block_number - 1) * substrate_block_time)
target_block_time = first_block_time + time_for_target_block

@ifrit98 ifrit98 self-requested a review January 4, 2024 16:36
Copy link
Contributor

@ifrit98 ifrit98 left a comment

Choose a reason for hiding this comment

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

LGTM. I'll take it from here

@ifrit98 ifrit98 merged commit b11753e into opentensor:staging Jan 4, 2024
9 checks passed
@saqib-codes-11 saqib-codes-11 deleted the feature/btcli-wallet-history branch January 4, 2024 20:00
@ifrit98 ifrit98 mentioned this pull request Jan 8, 2024
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.

3 participants