Skip to content
This repository has been archived by the owner on Nov 2, 2018. It is now read-only.

Add spend transaction fees to host financial metrics after completion… #3092

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add spend transaction fees to host financial metrics after completion… #3092

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions modules/host/storageobligations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. update the host's metrics too in 'threadedHandleActionItem'
  2. 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.

Copy link
Contributor Author

@nielscastien nielscastien Jun 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if sos == obligationFailed {
// Remove the obligation statistics as potential risk and income.
Expand All @@ -610,6 +613,9 @@ func (h *Host) removeStorageObligation(so storageObligation, sos storageObligati
// Add the obligation statistics as loss.
h.financialMetrics.LostStorageCollateral = h.financialMetrics.LostStorageCollateral.Add(so.RiskedCollateral)
h.financialMetrics.LostRevenue = h.financialMetrics.LostRevenue.Add(so.ContractCost).Add(so.PotentialStorageRevenue).Add(so.PotentialDownloadRevenue).Add(so.PotentialUploadRevenue)

// Add spend transaction fees
h.financialMetrics.TransactionFeeExpenses = h.financialMetrics.TransactionFeeExpenses.Add(so.TransactionFeesAdded)
}

// Update the storage obligation to be finalized but still in-database. The
Expand Down