-
Notifications
You must be signed in to change notification settings - Fork 10
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: AlgorandClient #71
Conversation
src/algokit_utils/__init__.py
Outdated
@@ -1,98 +1,71 @@ | |||
from algokit_utils._debugging import PersistSourceMapInput, persist_sourcemaps, simulate_and_persist_response |
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 project has pre-commit configured to run the various linting tools (black, ruff, mypy) https://github.com/algorandfoundation/algokit-utils-py/blob/main/.pre-commit-config.yaml.
You will need to run these and fix any issues to get the PR to pass the various checks, they will also help reduce some of the unnecessary noise in the changeset due to IDE formatting differences.
src/algokit_utils/account_manager.py
Outdated
def get_asset_information(self, sender: str, asset_id: int): | ||
return self._client_manager.algod.account_asset_info(sender, asset_id) | ||
|
||
# TODO |
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.
TODO?
src/algokit_utils/account_manager.py
Outdated
self.set_signer(account.address, account.signer) | ||
return AddressAndSigner(address=account.address, signer=account.signer) | ||
|
||
# TODO |
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.
Another TODO?
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 new AlgorandClient
class seems to do less than the old one. Can you argue for the benefits of using this new client instead of the current one?
self._default_validity_window: int = 10 | ||
|
||
def _unwrap_single_send_result(self, results: AtomicTransactionResponse) -> dict[str, Any]: | ||
return { |
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.
It would be nice to have typed return objects with dataclasses
src/algokit_utils/beta/composer.py
Outdated
self.txns.append(atc) | ||
return self | ||
|
||
def add_method_call(self, params: MethodCallParams) -> 'AlgokitComposer': |
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 don't understand why we are abandoning ad-hoc clients that offer you typed method calls in favor of generic clients where the user needs to know more about the arguments of the methods.
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're still expecting people to use generated clients for apps. This PR is mainly for addressing non-app related stuff. Wanted to support method calls for more advanced composition which is not currently possible with ATC (ie. nested method calls). Eventually the plan is to use this in the generated clients
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 don't see any code to attach the client to existing deployments and to perform idempotent deployments
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.
Yeah for now we are keeping app-related stuff in the existing ApplicationClient class. Eventually we want to refactor it to better fit with AlgorandClient
, but for now we're just focusing on stuff not covered by ApplicationClient
|
||
|
||
@dataclass | ||
class AddressAndSigner: |
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 wonder if it's possible to use an Account here.
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 reason I went with AddressAndSigner
is because Account
has a private key property, which is not applicable outside of localnet in most cases. Also I want to try to get away from Account
terminology and instead use Address
(with account being for HD wallet accounts that are used to derive addresses)
…pipx; adding dependabot; patching snapshot tests (#72) * fix: testing ci * chore: testing ci * chore: test * chore: test * chore: testing ci * chore: testing ci * chore: test * chore: testing * chore: test * chore: test * chore: removing tmp tweak * chore: testing ci * chore: lockfile maintenance (poetry update); reverting ci tweaks * chore: testing ci * chore: testing ci * chore: testing ci
@joe-p please remove the branch if no longer needed |
Proposed Changes
AlgorandClient
tobeta
namespace to mirror the AlgorandClient inalgokit-utils-ts
(and includes required classes)