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

MethodCallParams.Builder pulling v1 TransactionParams #323

Closed
deanstef opened this issue May 22, 2022 · 3 comments · Fixed by #324
Closed

MethodCallParams.Builder pulling v1 TransactionParams #323

deanstef opened this issue May 22, 2022 · 3 comments · Fixed by #324
Assignees
Labels
new-bug Bug report that needs triage Team Scytale

Comments

@deanstef
Copy link

Subject of the issue

The method setSuggestedParams() of the Java object MethodCallParams.Builder used for ABI calls with ATC is pulling TransactionParams from v1 API instead of v2's TransactionParametersResponse. These objects are serialized differently, as a consequence it is not possible to call ABI methods with the Java SDK.

Your environment

  • Software version: sandbox: 3.5.139117.dev

Steps to reproduce

TransactionParametersResponse sp = algod.TransactionParams().execute().body();

// create methodCallParams by builder (or create by constructor) for txntest method
MethodCallParams.Builder methodCallParamsBuilderTxn = new MethodCallParams.Builder();
// methodCallParams txn: commons params
...
methodCallParamsBuilderTxn.setSuggestedParams(sp);

Expected behaviour

Set suggested params pulled from algod v2 API as TransactionParametersResponse object

Actual behaviour

Getting error:
The method setSuggestedParams(TransactionParams) in the type MethodCallParams.Builder is not applicable for the arguments (TransactionParametersResponse)

@deanstef deanstef added the new-bug Bug report that needs triage label May 22, 2022
@deanstef
Copy link
Author

@winder do you mind to have a look at it?

@winder
Copy link
Contributor

winder commented May 22, 2022

@algoanne @michaeldiamant looks like this code would benefit from a similar approach to the transaction builder, which has helpers for the v1 and v2 API. See the two "suggestedParams" and "lookupParams" methods here.

Here is the method in question, which only supports the v1 API

I'm not familiar with the AtomicTransactionComposer, but my first impression is that the design for this feature overlaps with the preexisting transaction builders. Instead of using the "params" pattern, and a new builder convention why not add a new "MethodCallTransactionBuilder" class to the other builders?

Looking again this morning, it also seems that the MethodCallParams.Builder is missing at least one required common field:
genesis hash (or is that not required for method calls?)

@jasonpaulos
Copy link
Contributor

I've created PR #324 to fix this issue, mostly following @winder's suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-bug Bug report that needs triage Team Scytale
Projects
None yet
4 participants