Add detailed Soroban resource fee info in txmeta #175
Conversation
| union SorobanTransactionMetaExt switch (int v) | ||
| { | ||
| case 0: | ||
| void; | ||
| case 1: | ||
| SorobanTransactionMetaExtV1 v1; | ||
| }; | ||
|
|
||
| struct SorobanTransactionMeta | ||
| { | ||
| SorobanTransactionMetaExt ext; | ||
|
|
There was a problem hiding this comment.
What's driving the deep extension pattern in use here?
The layers of extensions at this level make all meta larger over time, and adds even more branches for ingesters to handle, especially if we continue the trend and keep growing.
I think we should make transaction meta extension v4, and it'd be more consistent with past decisions to try and batch at some layer all sub-changes (like what we did with preconditions) rather than have long extension trees. It'd mean any application parsing v4 would know they have a full and complete set of data.
There was a problem hiding this comment.
Alternatively if we want to keep the change isolated to the SorobanTransactionMeta, the pre-extension point can be used to make this struct a union, so that we have a SorobanTransactionMetaV1 and a SorobanTransactionMetaV2, which again would grow over time with less branches and without an extension tree.
There was a problem hiding this comment.
This is indeed debatable. The benefit of this approach is that optional data won't break the readers. There is also much less duplication, which not irrelevant for Core. E.g. doing a SorobanTransactionMetaV2 change now would be pretty disruptive for all the meta consumers, even though this is a purely incremental change that is not even useful for everyone. Modifying the ext version in the future is probably slightly less disruptive, but still would affect all the meta consumers.
I do agree though that the extension trees are pretty annoying to deal with. Maybe we could flatten them every once in a while, which is possible to do for meta - this would be a disruptive change, but at least we don't need to make such changes frequently and we'll have a better understanding of what is/isn't necessary.
There was a problem hiding this comment.
We've debated this pattern and those tradeoffs in the past and my understanding was that most folks leaned towards avoiding extension trees and identifying layers to make unions. (cc @stanford-scs @MonsieurNicolas)
While it forces updates at each change, it is conceptually easier to understand and results in the XDR being a better API which is worth investing in, and which more folks are consuming.
I don't think the avoiding disruption is actually material downstream, and the impact on complexity that the extension trees add is actually more disruptive.
There was a problem hiding this comment.
There are no trees yet, just an extension point. So I would suggest to proceed with the change as is in order to move faster - it would be great to start ingesting this data ASAP. We can do the cleanup when there is an actual 'disruptive' change that bumps a version of some union.
| int64 totalNonRefundableResourceFeeCharged; | ||
| // Total amount (in stroops) that has been charged for refundable | ||
| // Soroban resource fees. | ||
| // Currently this comprises the rent fee (`rentFeeCharged`) and the | ||
| // fee for the events and return value. | ||
| // Refundable resources are charged based on the actual resources usage. | ||
| // Since currently refundable resources are only used for the successful | ||
| // transactions, this will be `0` for failed transactions. | ||
| int64 totalRefundableResourceFeeCharged; | ||
| // Amount (in stroops) that has been charged for rent. | ||
| // This is a part of `totalNonRefundableResourceFeeCharged`. | ||
| int64 rentFeeCharged; |
There was a problem hiding this comment.
We could use XDR structure to communicate this break down more clearly by having a ResourceFee struct, and within it a non-refundable and refundable struct, with values captured in respective places.
### What Bump XDR used by env to pick up env meta changes. ### Why Picking up changes in stellar/stellar-xdr#175 ### Known limitations N/A
While it's in theory possible to compute the non-refundable and rent fee portions of the resource fee using the transaction and ledger diffs, it's way too cumbersome (especially in the rent fee case, as it requires evaluating all the ledger entry diffs).
This data also allows to compute the Soroban inclusion fee with simple math (
feeCharged - nonRefundableFee - refundableFee).Besides addressing some concerns raised stellar/stellar-core#4245, this also may be useful to the end user.