fix: missing transaction rejection reason #269
Conversation
Also reworked several custom exceptions.
Codecov Report
@@ Coverage Diff @@
## development #269 +/- ##
===============================================
+ Coverage 95.96% 96.03% +0.06%
===============================================
Files 41 41
Lines 1909 1915 +6
===============================================
+ Hits 1832 1839 +7
+ Misses 77 76 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks for finding the issue with rejection_reason in TransactionReceipt. 👍
This issue is however a little bit more complicated - we're trying to orient starknet.py around the RPC api since it will become a default way of interaction with starknet at some point. That's why simply changing the typing of the TransactionReceipt is not currently possible, see my comments where I outline a possible solution for this issue, let me know what you think.
I believe these changes could be done on our side if you're fine with us keeping the tests improvements added in this PR and modifying the rest.
| if response.code != StarkErrorCode.TRANSACTION_RECEIVED.name: | ||
| raise Exception("Failed to send transaction. Response: {response}.") | ||
| raise TransactionFailedError( | ||
| code=response.code, error_message="Transaction not received" | ||
| ) |
There was a problem hiding this comment.
When adding transactions through the RPC (which will become the default way sooner than later), there's no code returned at all, and we're planning on removing this if completely soon: the flow will then be to invoke a transaction and do a result.wait_for_acceptance().
| @@ -153,7 +153,7 @@ class TransactionReceipt: | |||
| block_number: Optional[int] = None | |||
| version: int = 0 | |||
| actual_fee: int = 0 | |||
| rejection_reason: Optional[str] = None | |||
| rejection_reason: Optional[Dict[str, Any]] = None | |||
There was a problem hiding this comment.
This cannot be changed that way since the RPC rejection_reason is a string not a dict. What could perhaps be done is to have a subclass like GatewayTransactionReceipt that adds a code field and keeping rejection reason as a string.
There was a problem hiding this comment.
IIR I encountered a validation error because it's effectively a dict.
There was a problem hiding this comment.
It is a dict in the gateway response but just a string in rpc. That's why I'd like to have rejection_reason as a string in TransactionReceipt (base class) since it's present in both, and then also have a code in GatewayTransactionReceipt. From my quick tests it seems like this dict only contains these two keys.
| def __str__(self): | ||
| return ( | ||
| f"Client failed{f' with code {self.code}' if self.code is not None else ''}" | ||
| f": {self.message}" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Error message is passed to the base Exception in super().__init__(self.message) so it should be printing correctly without overriding __str__.
There was a problem hiding this comment.
The interesting change I would like to keep here is the fact that .message only includes the message argument, and not a modified version appending "Client failed...". It would ease using the message attribute outside the project (else I would need to "parse" the message, and try to get back the original value, which is not really future-proof nor clean).
| @@ -161,7 +161,8 @@ async def wait_for_tx( | |||
| if result.block_number is not None: | |||
| return result.block_number, status | |||
| elif status == TransactionStatus.REJECTED: | |||
| raise TransactionRejectedError(result.rejection_reason) | |||
| reason = result.rejection_reason or {} | |||
| raise TransactionRejectedError(**reason) | |||
There was a problem hiding this comment.
Same with changes to the TransactionReceipt, we'd prefer to keep the TransactionRejectedError oriented to RPC which doesn't include code. We could add a subclass of this exception specifically for the gateway_client, that incudes the code along with the message, so it doesn't confuse the RPC users.
| @@ -114,8 +114,12 @@ class TransactionReceiptSchema(Schema): | |||
| block_number = fields.Integer(data_key="block_number", load_default=None) | |||
| version = fields.Integer(data_key="version", allow_none=True) | |||
| actual_fee = Felt(data_key="actual_key", allow_none=True) | |||
| rejection_reason = fields.String( | |||
| data_key="transaction_rejection_reason", allow_none=True, load_default=None | |||
There was a problem hiding this comment.
Just acknowledging that the data_key was incorrect in this case and since our tests didn't check error messages we didn't catch that it's not working correctly.
Of course, the sooner it's fixed the better. Am I getting it right if I say you will work on a proper patch on your side? If so, no worries, just go ahead :) |
Yes, we'd like to work on proper patch on our side. We'll however try to include the tests you added in this PR since they're very useful, so please keep the PR open for the time being. |
|
Closing this, because #271 was merged. Thanks for contributing! |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix a 0.4 regression about missing rejection reason from failed transactions.
I also reworked several custom exceptions, to homogenize with others, and to ease using exception attributes outside the project.
What is the current behavior? (You can also link to an open issue here)
No way to know what was the root cause of a failed transaction.
What is the new behavior (if this is a feature change)?
Now, the rejection reason is filled properly.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No. I changed internal stuff only.
Other information
It's a critical bug fix for Ape StarkNet, and SithSwap. Hopefully it will gain some attention to move forward quickly :)