event subscription shouldn't dismiss events from other phases

[gregdhill]

Signed-off-by: Gregory Hill [email protected]

[cla-bot-2021]

User @gregdhill, please sign the CLA here.

[ascjones]

I'm on holiday at the moment, back next week so can review properly then

[sander2]

@ascjones Any update on this?

[ascjones]

Apologies for the delayed review.

I believe the original intention of this was to only return those events which were triggered by the extrinsic.

Obviously the code has evolved since then with the option to wait for the events from the finalized block, for which I assume this is a fix.

However does this mean that the extrinsic filter does not work for subscribing to finalized block events? Because the index is only included in the ApplyExtrinsic phase.

As I understand, this means that subscribing with accept_weak_inclusion has different behaviour in that it can only return those events which are triggered by the extrinsic, whereas for finalized blocks events for all extrinsics will be returned.

In any case I'd like to see a test for this fix, if possible.

I think we also need to consider how to address the different semantics of subscribing to non-finalized vs finalized block events. It certainly makes the ExtrinsicSuccess API fairly useless, since it returns the first event which matches the event/module in that block. Think about how many Balances(Transfers) you might have...

[gregdhill]

the option to wait for the events from the finalized block, for which I assume this is a fix

We emit events in on_initialize and on_finalize for which this is a fix.

does this mean that the extrinsic filter does not work for subscribing to finalized block events?

It does work for extrinsics, the purpose of this PR was for block events from other phases.

this means that subscribing with accept_weak_inclusion has different behaviour

The behavior should be the same, in that both will return the appropriate events depending on whether the block has been finalized.

[sander2]

@ascjones can you merge either this pr or #306? We need it to be able to receive events posted in on_initialize hooks

[ascjones]

While this would fix the issue of filtering out events as part of Initialized and Finalized phases. I think the use case that this might break is e.g.

let result = api
        .tx()
        .balances()
        .transfer(dest, 10_000)
        .sign_and_submit_then_watch(&signer)
        .await?;

    if let Some(event) = result.find_event::<polkadot::balances::events::Transfer>()? {
        println!("Balance transfer success: value: {:?}", event.2);
    }

At the moment this will only give the Transfer event which was triggered by the extrinsic. If however an unrelated Transfer event is triggered in that block as part of, say, Initialized, then this event would be returned instead (correct me if I'm wrong).

Perhaps we could change the first block so that if the extrinisic filter is applied then it will still filter out the other phases. e,g,

if let Some(ext_index) = self.extrinsic {
    match phase {
        Phase::Initialization | Phase::Finalization => continue,
        Phase::ApplyExtrinsic(i) => {
            if i as usize != ext_index {
                continue
            }
        }
    }
}

Would that still work for your use case? (I assume listening for events in Initialization and Finalization without the self.extrinsic set.

https://github.com/paritytech/subxt/issues/309#issuecomment-960532382

In any case I think this api needs changing to expose the Phase and TransactionStatus to the user so they have finer grained control. See here: #309 (comment).

[ascjones]

Superseded by #321