fix(sequencer): move fee event recording to tx level #1718
Conversation
There was a problem hiding this comment.
I don't understand the patch notes: they are a very mechanical description of what was done but really explain why and to what effect. Can you explain what this means in terms of the execution flow? At which step of block production (in cometbft terms) are the events generated now?
If the event construction no longer happens as part of End Block, where does it happen now?
The patch notes also notes that this is a fix but it is not clear what is being fixed. Why is it a problem that the event is associated with the block and not the transaction? Weren't the sourceTransactionId and sourceActionIndex fields in the event intended to address this?
|
@SuperFluffy I attempted to update the patch notes to reflect the methodology behind the changes more. Let me know if this works for you. As for why I called it "fix", this is because it was pointed out as an issue that a transaction's fees were no longer being recorded with the transaction itself. That was the reason we added the source action and transaction id to the fee events though, so it may be a UX decision. I'll bring it up offline. |
@SuperFluffy Changing where the events are generated changes the external API for fees. You can no longer discover the fee paid by querying a transaction. You must now query the block and search for your tx hash in the results. Generally speaking this makes the block query return oversized payloads as now every query for a block returns the fees paid for every action on it. Fundamentally this a TX result not a block result. If we wanted a block result it should be the total fees paid over the whole block. |
| ("actionName", fee.action_name.to_string()).index(), | ||
| ("asset", fee.asset.to_string()).index(), | ||
| ("feeAmount", fee.amount.to_string()).index(), | ||
| ("sourceActionIndex", fee.source_action_index.to_string()).index(), |
There was a problem hiding this comment.
I actually think we should change this name to "positionInTransaction". I was part of the original review and had to do a double take what sourceActionIndex means.
There was a problem hiding this comment.
Followup issue here: #1735
Summary
Moves ABCI fee events to
FinalizeBlock.tx_resultsto identify fees on a per transaction basis.Background
Post-fee rework, fee ABCI event construction was happening outside of transaction execution and (wrongly) included in the
FinalizeBlock.eventsfield of the finalize block response. This made it difficult to identify which fee events were related to which transaction.This patch moves fee event construction back into the (atomic) transaction execution, so that the generated ABCI fee events per transaction are again part of
FinalizeBlock.tx_results.For example for Astrotek fees will again show up next to each transaction.
Changes
Testing
Adjusted fee event unit test to ensure fee event is returned as part of transaction execution.