polkascan / py-substrate-interface

Python Substrate Interface
https://polkascan.github.io/py-substrate-interface/
Apache License 2.0
240 stars 114 forks source link

Does not detect failed Sudo calls #214

Closed stanly-johnson closed 2 years ago

stanly-johnson commented 2 years ago

The receipt.is_success and receipt.error_message are all good even though the extrinsic failed, the error is within the Sudo module and the process_events() does not parse it. Is it meant to work this way?

Output of receipt.triggered_events

[<scale_info::16(value={'phase': 'ApplyExtrinsic', 'extrinsic_idx': 1, 'event': {'event_index': '0508', 'module_id': 'Balances', 'event_id': 'Withdraw', 'attributes': ('5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY', 2050039581502301)}, 'event_index': 5, 'module_id': 'Balances', 'event_id': 'Withdraw', 'attributes': ('5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY', 2050039581502301), 'topics': []})>, <scale_info::16(value={'phase': 'ApplyExtrinsic', 'extrinsic_idx': 1, 'event': {'event_index': '0200', 'module_id': 'Sudo', 'event_id': 'Sudid', 'attributes': {'Err': 'BadOrigin'}}, 'event_index': 2, 'module_id': 'Sudo', 'event_id': 'Sudid', 'attributes': {'Err': 'BadOrigin'}, 'topics': []})>, <scale_info::16(value={'phase': 'ApplyExtrinsic', 'extrinsic_idx': 1, 'event': {'event_index': '0507', 'module_id': 'Balances', 'event_id': 'Deposit', 'attributes': ('5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY', 2050039581502301)}, 'event_index': 5, 'module_id': 'Balances', 'event_id': 'Deposit', 'attributes': ('5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY', 2050039581502301), 'topics': []})>, <scale_info::16(value={'phase': 'ApplyExtrinsic', 'extrinsic_idx': 1, 'event': {'event_index': '0000', 'module_id': 'System', 'event_id': 'ExtrinsicSuccess', 'attributes': {'weight': 39547010000, 'class': 'Normal', 'pays_fee': 'Yes'}}, 'event_index': 0, 'module_id': 'System', 'event_id': 'ExtrinsicSuccess', 'attributes': {'weight': 39547010000, 'class': 'Normal', 'pays_fee': 'Yes'}, 'topics': []})>]
arjanz commented 2 years ago

I don't have more context, but from the events you provided, it seems like the extrinsic succeeded and not failed: The third event (triggered by extrinsic with index 1 in that block) is an ExtrinsicSuccess event.

Where did you see that your extrinsic failed? Did you perhaps saw events that are triggered by another extrinsic in the same block?

stanly-johnson commented 2 years ago

Sudo extrinsic is shown as Success but the actual execution failed, you can see this if you scroll along the logs {'event_index': '0200', 'module_id': 'Sudo', 'event_id': 'Sudid', 'attributes': {'Err': 'BadOrigin'}}

The process_events() misses it since the extrinsic is marked as Success, I am getting around by looking for it specifically like this

for event in receipt.triggered_events:
        if event.value['module_id'] == 'Sudo':
            if 'Err' in str(event.value['attributes']):

Again not sure if its meant to work like this, but adding this to the process_events() would be useful.

arjanz commented 2 years ago

Ok gotcha, I missed that attribute at first glance.

I can understand this seems not intuitive, but this is not something that should be altered in the library, because it is just translating the behaviour of this call in the Substrate runtime.

Apparently in the Sudo pallet a BadOrigin is interpreted as successful (executed with a valid Sudo account, but not allowed for given call). And for example a RequireSudo is a failure, because it is executed with an invalid Sudo account.

This is a design decision of that Substrate runtime pallet, so I believe best practice is to translate this as such to the applications interfacing with it in the most generic and unmodified way. Added exceptions in a library will just add confusing I'm afraid, because then behaviour wil differ from other libraries like PolkadotJS.

PolkadotJS is showing the same behaviour:

Screenshot 2022-05-05 at 17 35 04

Screenshot 2022-05-05 at 17 37 44

stanly-johnson commented 2 years ago

Makes sense. Thanks