-
Notifications
You must be signed in to change notification settings - Fork 12
Fixes for box population and errors from a rejected app call transaction #202
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
Conversation
031393e to
935b5f9
Compare
| "Fees were too small to resolve execution info via simulate. " | ||
| "You may need to increase an app call transaction maxFee." | ||
| ) | ||
| failed_at = group_response.get("failed-at", [0])[0] | ||
| details = "" | ||
| if "logic eval error" not in msg: | ||
| # extract last pc from trace so we can format an error that can be parsed into a LogicError | ||
| try: | ||
| trace = group_response["txn-results"][failed_at]["exec-trace"] | ||
| except (KeyError, IndexError): | ||
| pass | ||
| else: | ||
| try: | ||
| program_trace = trace["approval-program-trace"] | ||
| except KeyError: | ||
| program_trace = trace["clear-program-trace"] | ||
| pc = program_trace[-1]["pc"] | ||
| details = f". Details: pc={pc}" | ||
| raise ValueError( | ||
| f"Error resolving execution info via simulate in transaction {failed_at}: " | ||
| f"{group_response['failure-message']}{details}" |
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.
If these exceptions surface will it be confusing to developers, i.e. "Error resolving execution" -> "Why is it trying to "resolve execution, what does that mean?", or will these exceptions be clear that they are raised as part of resolving an error in a call?
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.
(Ah I see these are the existing error messages anyway, although the question still stands).
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.
They will be transformed into a LogicError (in the client error handling) which should map it to the appropriate TEAL or error message if they have an app spec
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.
Previously (on the TS side) this error was specific to resource population (Error populating resources...). When we added fee coverage support, the auto simulate behaviour would execute for that scenario too, so the error needed to be more generic.
|
These changes presumably also need to be made to typescript utils too |
yeah the one that adds the PC does. The other was python specific |
No description provided.