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

Improve confusing on_complete behaviour on application client calls #90

Closed
aorumbayev opened this issue May 13, 2024 · 2 comments
Closed
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@aorumbayev
Copy link
Contributor

aorumbayev commented May 13, 2024

Subject of the issue

This was reported by @cusma. The main reason is potential confusion that can be caused by the fact that user has ability to specify on complete in transaction parameters, yet its ignored in some scenarios like opt_in (where it overwrites the oncomplete to use OptInOC).

For instance:

    app_client.opt_in( # this internally is actually going to ignore the on_compete from user and set it to OptInOC
        transaction_parameters=OnCompleteCallParameters(
            on_complete=OnComplete.DeleteApplicationOC
        ),
    )

As seen above, the usage may be confusing, we either want to limit ability to use OnCompleteCallParameters (currently its a sub class of transaction params hence why it works) or account to prioritize taking the on_complete from transaction params if present.

Further notes

Based on discussion with @daniel-makerx and proposed fix in #79, the OnCompleteCallParameters and CreateTransactionParameters are no longer subclassing from common TransactionParameters.

@neilcampbell noted down issues with the propagated fix as they had additional implications on the generator-py repo that were not caught as part of the 2.2.2 release. Originally, the fix was propagated under 2.2.2, we then reverted the changes under 2.3.0 given that a heap of upstream changes would require a more thorough release coordination on beaker, beaker and python template, algokit-cli repos that we didn't account for when tagging this as a minor fix. Hence, after re evaluating the initial issue report and higher priority items around puya at the moment we decided to revert the #79 changes, log it as a proper github issue on the repo and address with proper cross repo release coordination and smoke testing. This is also due to the fact that a proper semantic tag would require a breaking change in version (v3) and this would also imply additional release coordination.

@cusma will ping you as soon as the bug is resolved properly.

@aorumbayev aorumbayev added the bug Something isn't working label May 13, 2024
@aorumbayev aorumbayev self-assigned this May 13, 2024
@aorumbayev aorumbayev added this to the Enhancements milestone May 13, 2024
@aorumbayev
Copy link
Contributor Author

@robdmoore this will be resolved as part of the update to new interfaces, will close the issue once its resolved

@aorumbayev aorumbayev added enhancement New feature or request and removed bug Something isn't working labels Jan 9, 2025
@robdmoore robdmoore changed the title Fix on_complete behaviour on application client calls Improve confusing on_complete behaviour on application client calls Jan 9, 2025
@aorumbayev
Copy link
Contributor Author

The new syntax in v3 of algokit-utils-py aligns with ts interfaces,

reported example would not look as such:

app_client.send.opt_in(params=AppClientMethodCallParams(on_complete=OnComplete.DeleteApplicationOC))

Unless on_complete is explicitly set in params, it will fill the expected OnComplete per operation otherwise it will respect the custom value provided as seen in

params.__dict__, on_complete=params.on_complete or algosdk.transaction.OnComplete.NoOpOC
.

Hence this addresses the previous behaviour where client would not respect custom values passed in on_completes for operations that were hardcoding expected values. (cc @cusma).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant