-
Notifications
You must be signed in to change notification settings - Fork 323
t8n: Return InvalidBlock
exception in result.blockException
#1182
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
t8n: Return InvalidBlock
exception in result.blockException
#1182
Conversation
except InvalidBlock as e: | ||
self.result.block_exception = f"{e}" |
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.
Should we hoist this up into:
def run_blockchain_test(self) -> None: |
?
We can wrap the whole function body in a try
/except
, since per-transaction exceptions are caught here.
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.
Yes that sounds better. Is run_blockchain_test
part of forks/prague
? I cannot see it in this version of the file.
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.
I believe so, yeah.
d615cb5
to
9b677f4
Compare
81aef27
to
d9af838
Compare
Just tested and it's not working anymore, I need to take another look. |
@SamWilsn I've rebased to forks/prague, applied your suggestion, fixed my code and now it's working finally. Let me know if it's ok. |
Just as a sanity check, I did a local check of the fixtures generated with:
And the resulting generated test fixtures were identical. |
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.
LGTM! The only nit would perhaps be to avoid the huge try-except block in run_blockchain_test
. Defining the logic in the try block as a separate function.
PS: Perhaps that was the original implementation.
What was wrong?
Block-level exceptions, such the ones raised when the system contract calls failed, resulted in t8n returning an empty response instead of parseable JSON.
How was it fixed?
Added a new field in the T8N's response's field
result
that is calledblockException
, which is populated in case of an error.In this PR only an exception in
process_general_purpose_requests
will be caught and populate the extra field, so every other exception is not caught and will still result in the same behavior, but I think each case should be analyzed separately.Cute Animal Picture