-
Notifications
You must be signed in to change notification settings - Fork 86
feat: Add set node account id method #362
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: main
Are you sure you want to change the base?
feat: Add set node account id method #362
Conversation
Signed-off-by: Angelina <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
…xamples Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
Signed-off-by: dosi <[email protected]>
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.
Hi @accepaluni please remember to pre-fix your PR title eg
feat: add set node account id method
additionally, we now have a changelog, which must be updated with each PR
|
Hi @aceppaluni, |
|
Hi @nadineloepfe Yes, my apologies. |
|
Reviewing this PR made me realise that we currently have a behavioural divergence between Query and Transaction: the former always selects the first node, while the latter uses a round-robin strategy via _last_used_index. whops. dw, this isn’t introduced by this PR, but it surfaced the inconsistency. I’ll raise a separate issue for this and keep it out of this PR though |
cd30c5d to
395fadf
Compare
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.
Hi @aceppaluni
One of your workflows threw an error
ERROR tests/integration/transfer_transaction_e2e_test.py - NameError: name 'List' is not defined
ERROR tests/integration/utils_for_test.py - NameError: name 'List' is not defined
ERROR tests/unit - NameError: name 'List' is not defined
Please check :)
thanks
094195f to
6abf051
Compare
|
@exploreriii I believe the error should be corrected now. |
b09233a to
2afb18d
Compare
|
|
||
| def set_node_account_id(self, node_account_id: AccountId): | ||
| """ | ||
| Selects a node account ID to use for sending a query. |
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 should be "sending a transaction"
|
|
||
| def _select_node_account_id(self) -> Optional[AccountId]: | ||
| """ | ||
| Selects a node account ID to use for sending a query. |
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 also should say "sending a transaction"
src/hiero_sdk_python/query/query.py
Outdated
| selected = self.node_account_ids[0] | ||
| self._used_node_account_id = selected | ||
| return selected | ||
| return None |
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.
could we end this file with a new line (python convcention)
CHANGELOG.md
Outdated
|
|
||
| ### Fixed | ||
| - Added explicit read permissions to examples.yml (#623) | ||
| - fix: spacing after set_node_account_id method (#362) |
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.
the changelog entry should be: "added node_account_ids method" or similar, not spacing
|
You need to modify the _execute method in executable.py to check if node_account_ids has been set and use also, please add an integration test (or raise a ticket so someone can pick this up later - executable as well as integration) :) |
591a4b6 to
ab18528
Compare
|
@nadineloepfe I will raise a ticket for the integration testing. :) |
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.
hi @aceppaluni note that the workflow has failed due to
ERROR tests/integration/token_pause_transaction_e2e_test.py::test_pause_sets_pause_status_to_paused - AttributeError: 'Network' object has no attribute '_get_node'. Did you mean: '_select_node'?
ab18528 to
df7a63a
Compare
|
@aceppaluni : in executable.py, the code calls But |
Signed-off-by: Angelina <[email protected]>
Signed-off-by: Angelina <[email protected]>
Signed-off-by: Angelina <[email protected]>
Signed-off-by: Angelina <[email protected]>
Signed-off-by: Angelina <[email protected]>
048ccc5 to
5f694bd
Compare
…election Signed-off-by: Angelina <[email protected]>
4d9bd11 to
6e60202
Compare
Description:
Add Set_node_account_ids() method to Query and Transaction for retrieving receipts or records.
Related issue(s):
Fixes #86
Notes for reviewer:
Ran tests for both Query and Transaction
Checklist