scs / substrate-api-client

Library for connecting to substrate API over WebSockets
Apache License 2.0
261 stars 124 forks source link

return error details upon included but failed extrinsic #138

Closed echevrier closed 3 years ago

echevrier commented 3 years ago

When the node send an error, we get only an ignoring unsupported module event. It would be good to have a nice way to get the error and listen to these errors

clangenb commented 3 years ago

If I understand correctly, you want to listen to an event that is triggered when the extrinsic fails?

Listening to custom events is supported. You can do it as follows:

https://github.com/integritee-network/worker/blob/25c2b69d127d1998d8ed6b3cc3ce6cb98efd2228/client/src/main.rs#L562-L587

echevrier commented 3 years ago

In this case we are waiting for the BlockConfirmed event. If the node throws an error and no BlockConfirmed event is sent, how can we catch the error?

haerdib commented 3 years ago

If the node throws an error before the extrinsic is finalized, the api client will return the error message (if you're listening until finalisation, like it's done here: https://github.com/integritee-network/worker/blob/25c2b69d127d1998d8ed6b3cc3ce6cb98efd2228/client/src/main.rs#L416-L418

This way you should atleast receive the error message once (there's a known issue of thread freezing #135 after receiving an error message, but the error log is printed either way) . Does this answer your question?

echevrier commented 3 years ago

Sorry I don't know. Here the description of my case:

I'm listening to a create event like this :

let tx_hash = chain_api
        .send_extrinsic(xt.hex_encode(), XtStatus::InBlock)
        .unwrap();
    //subscribe to event Created
    let (events_in, events_out) = channel();
    chain_api.subscribe_events(events_in).unwrap();

    //Wait for Created event to extract and return the NFTid
    let mut decoder = EventsDecoder::try_from(chain_api.metadata.clone()).unwrap();
    decoder.register_type_size::<NFTId>("NFTId").unwrap();
    decoder
        .register_type_size::<AccountId>("AccountId")
        .unwrap();
    decoder
        .register_type_size::<NFTSeriesId>("NFTSeriesId")
        .unwrap();

    let account_id = get_accountid_from_str(owner_ss58);
    debug!("AccountId of signer  {:?}", account_id);

    //For now no possibility to catch here the errors coming from chain. infinite loop.
    //See issue https://github.com/scs/substrate-api-client/issues/138#issuecomment-879733584
    loop {
        let ret = chain_api
            .wait_for_event::<CreatedArgs>("Nfts", "Created", Some(decoder.clone()), &events_out)
            .unwrap();

        info!("Created event received");
        if ret.account_id == account_id {
            return Some(ret.nft_id);
        }
    }   

If the node cannot create the NFT it will send an error (ex NotSeriesOwner in https://github.com/capsule-corp-ternoa/chain/blob/5ed166ae3f38b57c52dd4955d6b22c60347eddd2/pallets/nfts/src/lib.rs#L452). How can I catch this error? I found that something went wrong when I looked into the console and saw :

[2021-07-21T11:39:06Z DEBUG substrate_api_client::events] Decoding successful, system_event: ExtrinsicFailed(Module { index: 24, error: 4, message: None }, DispatchInfo { weight: 674000000, class: DispatchClass::Normal, pays_fee: Pays::Yes })
[2021-07-21T11:39:06Z DEBUG substrate_api_client::events] Phase Phase::ApplyExtrinsic(1), Event: System(ExtrinsicFailed(Module { index: 24, error: 4, message: None }, DispatchInfo { weight: 674000000, class: DispatchClass::Normal, pays_fee: Pays::Yes }))
[2021-07-21T11:39:06Z DEBUG substrate_api_client::events] Decoding topics [0]
[2021-07-21T11:39:06Z INFO  substrate_api_client] wait for raw event
[2021-07-21T11:39:06Z INFO  substrate_api_client] Decoded Event: Phase::ApplyExtrinsic(0), System(ExtrinsicSuccess(DispatchInfo { weight: 161650000, class: DispatchClass::Mandatory, pays_fee: Pays::Yes }))
[2021-07-21T11:39:06Z DEBUG substrate_api_client] ignoring unsupported module event: System(ExtrinsicSuccess(DispatchInfo { weight: 161650000, class: DispatchClass::Mandatory, pays_fee: Pays::Yes }))
[2021-07-21T11:39:06Z INFO  substrate_api_client] Decoded Event: Phase::ApplyExtrinsic(1), System(ExtrinsicFailed(Module { index: 24, error: 4, message: None }, DispatchInfo { weight: 674000000, class: DispatchClass::Normal, pays_fee: Pays::Yes }))
[2021-07-21T11:39:06Z DEBUG substrate_api_client] ignoring unsupported module event: System(ExtrinsicFailed(Module { index: 24, error: 4, message: None }, DispatchInfo { weight: 674000000, class: DispatchClass::Normal, pays_fee: Pays::Yes }))
haerdib commented 3 years ago

thewait_for_event function does not offer this functionality you're looking for, I agree. But I'm not sure if this function was ever meant to support multiple events. If you want to implement a specifc behaviour for different events, you can do this by using the subscribe_events() function (see the callback example). Or more verbous, directly in the client, implemented here: https://github.com/integritee-network/worker/blob/master/client/src/main.rs#L675 But you somehow need to ensure that you're not catching an error event meant for a different extrinsic.

I'm guessing the more fail safe approach would be to wait for block finalization instead of just InBlock, this way the api-client should (see issue #135) return an error if the extrinsic is not executed.

EDIT: Actually, no.. InBlock should already return an error if execution was not successful.

echevrier commented 3 years ago

The problem is not about the "wait for an event" function or the capture of multiple events, but the way we can "listen" to errors from substrate node. It would be nice to get them specifically and not to have to interpret them from the message

System(ExtrinsicFailed(Module { index: 24, error: 4, message: None }, ...
clangenb commented 3 years ago

@haerdib I believe this is not quite correct, we only get an error back when sending an extrinsic if it failed to get prevalidated and put into the tx_pool (e.g. insufficient funds, bad encoding, bad signature). If execution of an extrinsic fails within the pallet, as in this example, we need to listen for a very specific event: system.ExtrinsicFailed. However, to get the precise error is more involved. Here is the js-api equivalent of what needs to be done:

api.tx.balances
  .transfer(recipient, 123)
  .signAndSend(sender, ({ status, events }) => {
    if (status.isInBlock || status.isFinalized) {
      events
        // find/filter for failed events
        .filter(({ event }) =>
          api.events.system.ExtrinsicFailed.is(event)
        )
        // we know that data for system.ExtrinsicFailed is
        // (DispatchError, DispatchInfo)
        .forEach(({ event: { data: [error, info] } }) => {
          if (error.isModule) {
            // for module errors, we have the section indexed, lookup
            const decoded = api.registry.findMetaError(error.asModule);
            const { documentation, method, section } = decoded;

            console.log(`${section}.${method}: ${documentation.join(' ')}`);
          } else {
            // Other, CannotLookup, BadOrigin, no extra info
            console.log(error.toString());
          }
        });
    }
  });

I would first need to look at the api-client in more detail to figure out what needs to be done to get a similar behavior.

Beware: with the old pallet macro syntax: frame-v1, you needed to put the error in the decl_module! macro for it to be exported to the metadata. I don't know if there is a similar constriction in the frame-v2 syntax (the one the @echevrier uses in her NFT pallet.)

echevrier commented 3 years ago

In frame-v1, shall the error not be put in the decl_error! macro ? In frame-v2 would be #[pallet::error]. The NFT pallet I used does it. The errors are exported in the metadata.

clangenb commented 3 years ago

The errors are indeed declared in the decl_error! macro, but for them to be exported you also need to put them in the module:

https://github.com/integritee-network/pallet-teerex/blob/2b5ab50435b6dadcb033a562977196ccbe72f7ef/src/lib.rs#L117

As long as you see them in the metadata, it should be fine.

haerdib commented 3 years ago

@clangenb I see, thanks for the clarification. What's still not clear to me: Should this error catching / filtering be part of the api-client? Or is the user of the client responsible for catching these events?

clangenb commented 3 years ago

I would love the api-client to have this feature.