Add spend transaction fees to host financial metrics after completion… #3092
Conversation
… of storageobligation.
| @@ -596,6 +596,9 @@ func (h *Host) removeStorageObligation(so storageObligation, sos storageObligati | |||
| h.financialMetrics.StorageRevenue = h.financialMetrics.StorageRevenue.Add(so.PotentialStorageRevenue) | |||
| h.financialMetrics.DownloadBandwidthRevenue = h.financialMetrics.DownloadBandwidthRevenue.Add(so.PotentialDownloadRevenue) | |||
| h.financialMetrics.UploadBandwidthRevenue = h.financialMetrics.UploadBandwidthRevenue.Add(so.PotentialUploadRevenue) | |||
|
|
|||
| // Add spend transaction fees | |||
| h.financialMetrics.TransactionFeeExpenses = h.financialMetrics.TransactionFeeExpenses.Add(so.TransactionFeesAdded) | |||
There was a problem hiding this comment.
I don't think that this change is correct. It assumes that we never add the storage obligation's TransactionFeesAdded field to the host's financial metrics which is not correct. In fact we already add that field to the storage obligation when it is created or modified.
The real problem seems to be that we potentially modify the so.TransactionFeesAdded field after adding it to the host's financial metrics in 'threadedHandleActionItem', resulting in the host's financial metrics to no longer being accurate.
I would like to hear what @DavidVorick and @lukechampine think about it, but I think there are 2 solutions to this.
- update the host's metrics too in 'threadedHandleActionItem'
- change the host's metrics to also have a 'PotentialTransactionFeeExpenses' field and only set the value of the original field when we remove the obligation the same way we do it for the other metrics.
Either way we would need to add a test that confirms that the host's metrics are accurate. Looks like this is not currently covered by our test suit.
There was a problem hiding this comment.
@ChrisSchinnerl thanks for reviewing! You are right that there are several places where TransactionFeesAdded are added to the host's financial metrics. The problem is that these fields are always zero. This is true for new contracts as well as for renewed contracts.
The current PR adds the transaction fees at the end when the obligation is removed from the host's database. This is the simplest way and we can remove the other lines where the host's financial metrics are updated. The downside is that the value would be lagging.
I think the proposal to update of the financial metrics during threadedHandleActionItem is a better solution. If there is consensus about how to update the value I will change and resubmit the PR.
There was a problem hiding this comment.
Yeah lagging values might be confusing. Depending on @DavidVorick 's plans for the host module, your solution + removing the other lines might be the easiest one if we want to completely overhaul the host module in the future anyway.
If we don't want to overhaul all of it, I would recommend to update the metrics in 'threadedHandleActionItem' and also remove the lines to avoid confusion.
… of storageobligation.