Skip to content

feat: send payment transaction with algokit core #159

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

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

CiottiGiorgio
Copy link
Member

(draft) PR to use AlgoKit Core when dealing with a Payment transaction in utils.

Companion PR of algorandfoundation/algokit-utils-ts#393

@CiottiGiorgio
Copy link
Member Author

At this point, the local AlgoKit Core this is using is from algorandfoundation/algokit-core#81

@CiottiGiorgio CiottiGiorgio changed the title WIP: Send and wait payment transaction with algokit core Send payment transaction with algokit core May 15, 2025
Copy link
Contributor

@aorumbayev aorumbayev left a comment

Choose a reason for hiding this comment

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

some minor comments, would also be keen to see how it behaves in CI, would be interesting to compare execution time for tests on version with and without these changes. I think you need to patch merge conflict first to have workflows executed

@CiottiGiorgio CiottiGiorgio changed the title Send payment transaction with algokit core feat: send payment transaction with algokit core May 16, 2025
@CiottiGiorgio
Copy link
Member Author

At this point, I have added the ability for utils to send all transactions through the generated API client. I had initially aimed for just payments but I got carried away and did it for everything.
Also, the header update is a dirty trick for now while I understand how to use the api key correctly.

@aorumbayev
Copy link
Contributor

aorumbayev commented May 22, 2025

@CiottiGiorgio i noticed tests are failing in resource packing suite. The root cause is due to the fact that now calls are done via our oas client, while it throws similar errors from algod its a different exception, you'd need to adjust error handling in logic_error.py, to ensure regex searches over entire error string

    match = re.search(LOGIC_ERROR, error_str, re.DOTALL) # add re.DOTALL, and change re.match to re.search

and the exception handlers would also need to catch the error class from new oas client

under transaction_composer.py's send method

  except algosdk.error.AlgodHTTPError as e:
            raise Exception(f"Transaction failed: {e}") from e

should handle the algokit_algod_api.exceptions.BadRequestException as well

@aorumbayev
Copy link
Contributor

@CiottiGiorgio looks like you need to do poetry update setuptools to fix pip audit step. The non pypi package might need to be excluded explicitly as well (if you run it locally you can confirm if its a warning or treated as an error as well)

@CiottiGiorgio CiottiGiorgio force-pushed the feat/algokit-core-payment branch from b3d7b99 to 481f712 Compare May 29, 2025 11:30
api_client = algokit_algod_api.ApiClient(configuration)
self._algod_core_client = algokit_algod_api.AlgodApi(api_client=api_client)

def send_raw_transaction(self, txn):
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI in utils-ts we're only sending payment transactions via the new core client.
In theory it should be fine to send all transactions though.

import algosdk.transaction


def build_payment_with_core(
Copy link
Contributor

@neilcampbell neilcampbell May 29, 2025

Choose a reason for hiding this comment

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

This logic has been updated in utils-ts to also perform fee calculations via algokit-transact. See algorandfoundation/algokit-utils-ts#407

…g core algod client even though at this point tests are only using the non-experimental version of this package
@CiottiGiorgio
Copy link
Member Author

CiottiGiorgio commented May 30, 2025

At this point I think this PR is mostly good to go minus:

  • fee estimation should be done with core
  • more care in evaluating whether the core can send all transactions instead of just payments
  • the failing tests are due to the latest localnet where the node returns something different wrt boxes

I'll point out that I've explicitly told poetry to test the non-experimental version of this package in CI/CD and that I've also silenced a lot of mypy/ruff errors in the bridge code relating to how the signatures are untyped (since algosdk signatures are also untyped, I thought this was ok).
The experimental version on this package still passes the same tests that the non-experimental version passes (minus the ones that are at the moment related to the new algod box API).

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