-
Notifications
You must be signed in to change notification settings - Fork 69
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
Introduce unified MethodCallTransactionBuilder
& deprecate MethodCallParams.Builder
#324
Conversation
src/main/java/com/algorand/algosdk/builder/transaction/MethodCallTransactionBuilder.java
Show resolved
Hide resolved
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.
@jasonpaulos Thanks for your effort to supply a fix here! ☕
It's the 1st time I've substantively interacted with java-algorand-sdk, so take my approval with a grain of salt.
- I think the PR definitively addresses the bug and provides a solution for build conformance.
- Noting a passing concern - Generally, I am fearful of inheritance chains because they often increase fragility. I'm not requesting a change, but I feel obligated to vocalize the concern as a reviewer.
- I don't have enough experience with the repo to gauge the level of concern with
TransactionParametersBuilder
. - The inheritance is an expedient way to provide the same API. From a brief scan, I don't immediately see what other abstract classes may appear.
- I don't have enough experience with the repo to gauge the level of concern with
@jasonpaulos Approval note missed in previous comment: Since I don't yet have a well-defined view here, I'm trusting the provided level of backwards compatibility is in conformance with repo conventions + norms. |
src/main/java/com/algorand/algosdk/builder/transaction/MethodCallTransactionBuilder.java
Show resolved
Hide resolved
|
||
import com.algorand.algosdk.crypto.Address; | ||
|
||
public interface ApplicationCallReferencesSetter<T extends ApplicationCallReferencesSetter<T>> { |
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.
thanks for the discussion #324 (comment), and now I understand the reason behind the syntax T extends ...<T>
among all the txn builder.
This PR fixes a few related issues with
MethodCallParams.Builder
:setAppID
vsapplicationId
)setSuggestedParams
only accepted a v1TransactionParams
object, not a v2TransactionParametersResponse
object)To remedy these, I've followed @winder's suggestion to make a new
MethodCallTransactionBuilder
class, similar to the other transaction builder classes. To make sure this class uses the same methods as the other builders, I've done the following:TransactionBuilder
andMethodCallTransactionBuilder
inherit from a newTransactionParametersBuilder
class, which contains all common transaction parameters and their various setters.StateSchemaSetter
,TEALProgramSetter
, andApplicationCallReferencesSetter
. These interfaces define setter methods for various application call transaction fields, and their purpose is to unify these setters between the application call transaction builders and the newMethodCallTransactionBuilder
.And to preserve backwards computability as much as is reasonable, the original
MethodCallParams.Builder
class still exists as an adapter between the preexisting interface and the newMethodCallTransactionBuilder
class, though it is now deprecated.Backwards incompatible changes are:
MethodCallParams.Builder
's members are no longer public.MethodCallParams
's members have changed to reflect the internal types of the newMethodCallTransactionBuilder
, and have becomefinal
. Its constructor has changed accordingly.Closes #323