-
Notifications
You must be signed in to change notification settings - Fork 63
Feat/proxy additions 742 #745
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
base: staging
Are you sure you want to change the base?
Conversation
PRs need to be opened against the staging branch. |
thewhaleking
left a comment
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.
This looks really good overall. Please just fix that json_output comment. I'll test thoroughly later today, and if it's good will merge.
thewhaleking
left a comment
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.
looks good overall. just fix the top level imports issue, and this should include e2e test (can be slotted into the other test_proxy.py tests, rather than needing to make a brand-new dedicated test)
Okay, fixed the issue and included e2e test. |
thewhaleking
left a comment
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.
Your merge conflict resolution completely overwrites #748
Implements the missing proxy commands as outlined in issue opentensor#742: - Add 'btcli proxy list' command to query and display all proxies for an account - Add 'btcli proxy reject' command to reject announced proxy transactions - Add '--all' flag to 'btcli proxy remove' to remove all proxies at once All proxy functions properly use confirm_action with decline/quiet parameters to support the --no flag feature from PR opentensor#748. Includes comprehensive unit tests (22 tests) covering: - Success cases, JSON output, error handling - Prompt declined scenarios, wallet unlock failures - CLI command routing tests
481c0d4 to
ca75bab
Compare
- Add decline parameter for naming consistency - Pull announcements from ProxyAnnouncements table - Mark as executed after successful rejection - Allow delegate to default to wallet's coldkey - Support interactive selection when multiple announcements exist
- Make list_proxies more robust by normalizing proxy data keys - Add JSON output for all error paths in proxy_reject CLI
…ert bytes to SS58
…on external commands
|
@thewhaleking I think that the team will likely be mostly off next week, I hope this can be the last PR for now. |
thewhaleking
left a comment
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.
failing tests
- Fix test_list_proxies_* tests to mock subtensor.query() instead of substrate.query since list_proxies uses the former - Fix test_reject_announcement_with_prompt_declined to assert False instead of None (function returns bool, not None) - Fix test_proxy_reject_calls_reject_announcement to mock ProxyAnnouncements.get_db to avoid sqlite3 database access in CI
thewhaleking
left a comment
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.
I've had to unresolve a bunch of your resolved comments because you, for some reason I cannot quite determine, have just gone and re-added them back in.
The imports got re-added inside the test functions during a merge conflict resolution. I've now moved them all to top-level as originally requested. The fix has been pushed and all tests are passing locally. |
|
All test have passed now. Please let me know the result on your side. |
|
@thewhaleking Any chance you could review the update? I'd love to get this one wrapped up before the holidays if possible. |
I'll take a look when I get a chance, but it's a very large PR. |
feat: Add Proxy List, Reject Commands and Remove --all Flag
Summary
This PR implements the missing proxy commands as outlined in issue #742.
Closes #742
Changes
New Commands
btcli proxy listLists all proxies configured for an account by querying the chain's
Proxy.Proxiesstorage.Output includes:
btcli proxy rejectRejects a previously announced proxy call by calling the
Proxy.reject_announcementextrinsic. This allows the real account (proxied account) to cancel an announced transaction before it can be executed.Parameters:
--delegate: The SS58 address of the delegate who made the announcement--call-hash: The hash of the announced call to rejectModified Commands
btcli proxy remove --allAdded
--allflag to remove all proxies at once by calling theProxy.remove_proxiesextrinsic.Note:
--alland--delegateare mutually exclusive.Implementation Details
Files Changed
bittensor_cli/cli.py- Added CLI command handlers forproxy_list,proxy_reject, and updatedproxy_removebittensor_cli/src/commands/proxy.py- Added core async functions:list_proxies()- Queries chain storagereject_announcement()- Submits reject_announcement extrinsicremove_all_proxies()- Submits remove_proxies extrinsicSubstrate Proxy Pallet Extrinsics Used
proxy listProxy.Proxies[address]proxy rejectProxy.reject_announcementdelegate,call_hashproxy remove --allProxy.remove_proxiesTesting
Unit Tests Added (18 new tests)
list_proxiestests:test_list_proxies_success- Verifies correct chain query and table displaytest_list_proxies_json_output- Verifies JSON output formattest_list_proxies_empty- Handles empty proxy list gracefullytest_list_proxies_error_handling- Handles query errorsremove_all_proxiestests:test_remove_all_proxies_success- Verifies extrinsic composition and successtest_remove_all_proxies_with_prompt_declined- User can cancel operationtest_remove_all_proxies_unlock_failure- Handles wallet unlock failurereject_announcementtests:test_reject_announcement_success- Verifies extrinsic composition and successtest_reject_announcement_json_output- Verifies JSON output formattest_reject_announcement_with_prompt_declined- User can cancel operationtest_reject_announcement_failure- Handles extrinsic failureCLI integration tests:
test_proxy_remove_all_and_delegate_mutually_exclusive- Validates flag exclusivitytest_proxy_remove_requires_delegate_or_all- Validates required parameterstest_proxy_remove_with_all_flag_calls_remove_all_proxies- Verifies correct function calledtest_proxy_remove_with_delegate_calls_remove_proxy- Verifies existing behavior preservedtest_proxy_list_with_address- Verifies address parameter handlingtest_proxy_list_without_address_uses_wallet- Verifies wallet fallbacktest_proxy_reject_calls_reject_announcement- Verifies correct function calledRunning Tests
Lint Check
ruff check bittensor_cli/src/commands/proxy.py tests/unit_tests/test_cli.py # All checks passed!Checklist
proxy listcommandproxy rejectcommandproxy remove --allflag--json-output--no-promptfor scriptingContribution by Gittensor, learn more at https://gittensor.io/