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

Comment as op #410

Merged
merged 15 commits into from
Aug 16, 2022
Merged

Comment as op #410

merged 15 commits into from
Aug 16, 2022

Conversation

barnjamin
Copy link
Contributor

@barnjamin barnjamin commented Jun 22, 2022

Alternate impl to #407

for Comment(Assert(Int(1)), "yep")

#pragma version 6
int 1
assert
// yep
int 1
return

Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

I also made a child PR which changes the Comment implementation: #492

pyteal/ast/__init__.py Outdated Show resolved Hide resolved
pyteal/ast/__init__.py Outdated Show resolved Hide resolved
pyteal/ast/comment_test.py Outdated Show resolved Hide resolved
@jasonpaulos jasonpaulos mentioned this pull request Aug 3, 2022
@barnjamin barnjamin marked this pull request as ready for review August 4, 2022 10:36
@barnjamin
Copy link
Contributor Author

barnjamin commented Aug 4, 2022

With this pr, adding comments on asserted expressions, parsing the error msg from algod out, then mapping it back to source teal I get the following output:

Txn WWVF5P2BXRNQDFFSGAGMCXJNDMZ224RJUGSMVPJVTBCVHEZMOMNA had error 'assert failed pc=883' at PC 883 and Source Line 579: 

        store 50
        store 49
        store 48
        store 47
        // correct asset a
        load 50
        txnas Assets
        bytec_0 // "a"
        app_global_get
        ==
        assert          <-- Error
        // correct asset b
        load 51
        txnas Assets
        bytec_1 // "b"
        app_global_get
        ==
        assert
        // correct pool token
        load 49

This is v close to what I think we'd want but the comment is a little far from the actual assert. I think the ideal output is that the comment is on the same line as (or just above) the assert so its easier to see what we're asserting and even provide a friendlier error message on assert. wdyt?

Maybe a different Expr for assert w/ message is warranted?

Copy link
Member

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

The specific case of Assert aside, I think this is a good addition by itself.

Note: probably we should get another approval before merging, as I also contributed to this PR.

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.

generally lookin good, and I like the PR number, for that is my birthday.

pyteal/ir/ops.py Outdated Show resolved Hide resolved
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.

Notice, should also add an entry in CHANGELOG.md, and consider a bit on #509

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.

free approval bot says approve

@barnjamin barnjamin merged commit cabe6b8 into master Aug 16, 2022
@barnjamin barnjamin deleted the comment-as-op branch August 16, 2022 16:09
@jasonpaulos jasonpaulos mentioned this pull request Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants