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

Fix OnCompletion flag for goal app method #3228

Merged
merged 3 commits into from
Nov 19, 2021

Conversation

jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented Nov 18, 2021

Summary

Fix the --on-completion flag for goal app method, introduced in #3088.

Test Plan

Added test to test/scripts/e2e_subs/e2e-app-abi-add.sh

@ahangsu
Copy link
Contributor

ahangsu commented Nov 18, 2021

Looks pretty legit. I think it should be good to go once test (and related e2e test) passed.

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2021

Codecov Report

Merging #3228 (313e0a5) into master (4634e98) will increase coverage by 0.01%.
The diff coverage is 18.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3228      +/-   ##
==========================================
+ Coverage   47.45%   47.46%   +0.01%     
==========================================
  Files         369      369              
  Lines       59538    59540       +2     
==========================================
+ Hits        28251    28259       +8     
+ Misses      28005    28001       -4     
+ Partials     3282     3280       -2     
Impacted Files Coverage Δ
cmd/goal/application.go 12.89% <18.18%> (+0.11%) ⬆️
data/transactions/verify/txn.go 43.42% <0.00%> (-0.88%) ⬇️
network/requestTracker.go 70.25% <0.00%> (-0.87%) ⬇️
catchup/service.go 69.82% <0.00%> (ø)
network/wsPeer.go 68.88% <0.00%> (+3.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4634e98...313e0a5. Read the comment docs.

@jasonpaulos jasonpaulos marked this pull request as ready for review November 19, 2021 15:47
@jasonpaulos jasonpaulos requested a review from jannotti November 19, 2021 15:52
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

This looks good. If I'm understanding properly, we only supported on-completion for app create before? Why not for normal app calls?

That's certainly a little outside what we're doing with the ABI work right now, but would it be reasonable to add the flag to that as well?

@jasonpaulos
Copy link
Contributor Author

jasonpaulos commented Nov 19, 2021

The --on-completion flag was previously used only for goal app create because every OnCompletion value was baked into the other subcommands. E.g. goal app call implies NoOp, goal app optin implies OptIn, etc.

Maybe there is value in supporting it for goal app call, but really it's just as easy to use the subcommand name as the OnCompletion value.

@jannotti
Copy link
Contributor

The --on-completion flag was previously used only for goal app create because every OnCompletion value was baked into the other subcommands. E.g. goal app call implies NoOp, goal app optin implies OptIn, etc.

Maybe there is value in supporting it for goal app call, but really it's just as easy to use the subcommand name as the OnCompletion value.

I see, yeah, that's true, as long as goal app optin supports all the usual arg and foreign array building flags.

@jannotti jannotti merged commit 0b0f97b into algorand:master Nov 19, 2021
@jasonpaulos jasonpaulos deleted the on-complete-goal-app-method branch November 19, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants