-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add missing fields to InnerTx in runtime #761
base: develop
Are you sure you want to change the base?
Conversation
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.
utACK,
Yes, it was forgotten. We will check if other fields are missing.
We will backport it to v5.x release |
Thanks! If you are working on it, maybe it would make sense to provide stub support for TEAL 7 (it's in the same file, not sure if it may touch others)? Similarly to what Algobuilder has for TEAL 6, i.e. See #759 for some context. |
Hmm maybe it actually breaks something... I found myself in a situation where Probably it is related to this change, but not 100% sure. |
Yes, it is wrong, but I know how to do it properly, I will update this MR later. Actually, it should be handled differently. It is an int array, and there are no handling for int arrays yet, only byte arrays and ints. The good news is that it will probably handle other cases such as ForeignApps with no additional effort. |
da40082
to
c95775f
Compare
I implemented a proper fix and added it to tests (modified tests were failing before the fix). I am going to add |
Accounts are done. I don't see anything [important] missing now. I feel like it can be merged and backported @robert-zaremba PTAL |
@robert-zaremba friendly ping |
Don't mean to be intrusive but could somebody take a look? It's blocking me and I have to change compiled js to make algobuilder work for my project which is quite inconvenient. |
Same problem here. I think I'll use your branch. TY. |
Hey @MetaB0y . I git-cloned your forked repo and switched to branch Do you want to fix it or should I make a new PR? |
@PabloLION hi, I am not sure if it was changed recently but I don't think there was version 7 in algobuilder runtime at all. You can see it in the commit history. And even if there is version 7, isn't it supposed to copy values from Regarding making a new PR - I don't mind, I just want this change to be merged, it doesn't matter how :) |
Yea sorry. I didn't follow up this thread. I realised that algo-builder supports up to pyteal 0.15 or 0.13, which supports up to teal v6. I tested your PR by adding building it and using it with my own test cases, after copying all v6 to v7 like you said, "copy values from 6 and extend it". And it worked for everything I needed. Although I didn't run the tests of this repo, I think this is a good PR. |
It works the same way as others, I don't see any reason why it was not included previously; I assume it was forgotten?
Possible follow-ups: