Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

allow app id to be none since it may be called during create #592

Merged
merged 4 commits into from
Nov 3, 2022

Conversation

barnjamin
Copy link
Contributor

While answering a question in the forum, I realized I'd not considered the case where a MethodCall may be used to create an application.

Since the AVM doesn't like the manual setting of AppId 0, we should allow app_id to be None and prevent setting the app id field

@jasonpaulos
Copy link
Contributor

Good catch. The solution looks good but it'd be nice to see a regression test added to test_InnerTxnBuilder_method_call

Copy link
Contributor

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. I'm expecting mypy to flag the fact that app_id could now be None. So, I recommend changing its signature to:

app_id: Optional[Expr]

@ahangsu
Copy link
Contributor

ahangsu commented Nov 3, 2022

looks good, I will approve after a test on app-id == None gets added

@ahangsu
Copy link
Contributor

ahangsu commented Nov 3, 2022

also add a line in CHANGELOG Fixed section about this fix

@barnjamin barnjamin requested a review from tzaffi November 3, 2022 19:18
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

looks right to me

Co-authored-by: Zeph Grunschlag <tzaffi@users.noreply.github.com>
@barnjamin barnjamin merged commit 38934d2 into master Nov 3, 2022
@barnjamin barnjamin deleted the allow-app-id-none branch November 3, 2022 19:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants