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

Expose ATC transactions field with a getter method #759

Open
barnjamin opened this issue Apr 5, 2023 · 5 comments
Open

Expose ATC transactions field with a getter method #759

barnjamin opened this issue Apr 5, 2023 · 5 comments
Labels
new-feature-request Feature request that needs triage Team Scytale

Comments

@barnjamin
Copy link
Contributor

barnjamin commented Apr 5, 2023

Problem

Right now the ATC holds an array of TransactionWithSigner objects that is private

https://github.com/algorand/js-algorand-sdk/blob/develop/src/composer.ts#L120

This means to get access to the transactions, the developer must call buildGroup which changes the state of the ATC.

It is possible to clone the ATC prior to calling buildGroup so that the state of the original ATC is unchanged but this is not obvious.

Solution

Add a getter method to allow access to the transactions constructed so far

@barnjamin barnjamin added the new-feature-request Feature request that needs triage label Apr 5, 2023
@jasonpaulos
Copy link
Contributor

I think it's in our best interests to keep all implementation fields private. This prevents users from modifying any internal state in unexpected ways, and it makes maintenance and future code changes easier for us.

However, if you wanted to add a getter method to access individual transactions, that's probably reasonable.

@barnjamin barnjamin changed the title Expose ATC transactions field as protected Expose ATC transactions field with a getter method Apr 5, 2023
@robdmoore
Copy link

Ben raised this after I provided some feedback to him about how ATC has a bunch of useful logic, but it's not very flexible for extension, hiding most of it away.

Agreed making protected isn't right, but I'd like to request the following two changes which would significantly improve the flexibility / extensibility:

  • Return the transaction(s) / method call(s) that a call to addMethodCall creates (it may add multiple transactions, if they aren't returned then you have to do a .clone().buildGroup() to get the transactions every time you call it which is tedious and inefficient)
  • Return the PendingTransactionInfo from execute (otherwise if you want that object you have to issue an additional algod call to get that data)

@jasonpaulos
Copy link
Contributor

Thanks for that insight @robdmoore. I just have a few questions:

  • Return the transaction(s) / method call(s) that a call to addMethodCall creates (it may add multiple transactions, if they aren't returned then you have to do a .clone().buildGroup() to get the transactions every time you call it which is tedious and inefficient)

This seems like a reasonable request. Can I ask why you're interested in seeing the transactions produced by addMethodCall though?

  • Return the PendingTransactionInfo from execute (otherwise if you want that object you have to issue an additional algod call to get that data)

We currently do return the PendingTransactionInfo for all method calls from execute. They're accessible in ABIResult.txInfo. Is this what you're looking for, or do you also want PendingTransactionInfo for non-method calls?

@robdmoore
Copy link

Apologies for the delay in replying.

I want to see the transactions from addMethodCall because I have a wrapper that provides specific functionality that results in a call to addMethodCall and one of the return values from this method is the list of transaction(s) it generated. As an escape hatch and to allow for more advanced chaining, this method optionally takes an Atomic Transaction Composer so I can't just grab every single method in it since it may already have some methods.

Here's some relevant lines with one example: https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/app-deploy.ts#L198 and https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/app-deploy.ts#L234.

Yes - PendingTransactionInfo is what we want, I didn't realise it was available in the ABIResult. I do also want it for non-ABI calls too for consistency. Currently, I need to reissue HTTP requests to get this info per: https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/transaction.ts#L182-L187

https://github.com/algorandfoundation/algokit-utils-ts/blob/main/src/transaction.ts#L147

@jasonpaulos
Copy link
Contributor

I agree it definitely makes sense to do these things:

  • Add a method to the AtomicTransactionComposer to get individual unsigned transactions. This is a read-only operation, it can be used immediately after adding a transaction to the composer, and it doesn't change the status at all.
  • Change the return type of AtomicTransactionComposer.addMethodCall from void to Transaction. The returned value would be the app call transaction created by addMethodCall.

About returning pending transaction info for non-method calls from AtomicTransactionComposer.execute: it isn't really any more efficient for the composer to do inside of its execute method than it is for the caller to do this. That's because a separate GET request is necessary for each transaction in the group, and right now the composer only GETs method call transactions. So there is no argument for efficiency here (in fact it'd probably be less efficient if we started doing this and callers didn't care about the results), but I could see some convenience/consistency benefits to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature-request Feature request that needs triage Team Scytale
Projects
None yet
Development

No branches or pull requests

5 participants
@barnjamin @robdmoore @jasonpaulos @Eric-Warehime and others