GetFee uses the DollarInRune value (which gives runeUSDPrice) as though it were its inverse
Context (seeing double-expected outbound fees after TorAnchor Mimir keys were set): This Discord thread.
anchorMedian
(used for DollarInRune
) has this line:
value := pool.RuneValueInAsset(cosmos.NewUint(constants.DollarMulti * common.One))
In other words, it returns (and DollarInRune
takes the median of) an asset amount, the asset per 1 RUNE.
The function name DollarInRune
would be clearer if it were DollarsPerRune
,
as DollarInRune
suggests 1 USD in terms of RUNE
(while being the inverse).
This comment and naming is correct:
// get 1 RUNE price in USD
runeUSDPrice := telem(DollarInRune(ctx, mgr).QuoUint64(constants.DollarMulti))
telemetry.SetGauge(runeUSDPrice, "thornode", "price", "usd", "thor", "rune")
Indeed, DollarInRune
gives the USD price of RUNE.
[Edit: This comment for DollarInRune
is thus backwards and should be changed.
// gets the amount of rune that is equal to 1 USD
]
Now, GetFee
. (Used both for the fee itself and for the /inbound_addresses querier endpoint reporting the fee).
Here:
if gm.mgr != nil {
oneDollarRune = DollarInRune(ctx, gm.mgr).QuoUint64(constants.DollarMulti)
}
minAsset := cosmos.ZeroUint()
if !oneDollarRune.IsZero() {
// since MinOutboundUSD is in USD value , thus need to figure out how much RUNE
// here use GetShare instead GetSafeShare it is because minOutboundUSD can set to more than $1
minOutboundInRune := common.GetUncappedShare(cosmos.NewUint(uint64(minOutboundUSD)),
cosmos.NewUint(common.One),
oneDollarRune)
minAsset = pool.RuneValueInAsset(minOutboundInRune)
}
GetUncappedShare
takes arguments
(part, total, allocation)
and returns
allocation / (total / part)
, being approximately
allocation * (part / total)
.
(This cancels out the shared scale of part and total which have the same units,
leaving allocation (the amount to be divided up or derived from) to be divided by a unitless ratio.)
As RuneValueInAsset
is used to convert minOutboundInRune to minAsset
,
so minOutboundInRune is assumed to be in units of RUNE.
part: minOutboundUSD is in units of USD.
allocation: oneDollarRune (a misleading name) is the USD price of 1 RUNE, and is also in units of USD.
total: this in the denominator is 1, and to give a RUNE-units output would have to be USD squared per RUNE.
Instead, I propose that oneDollarRune should instaed be named runeUSDPrice and be 'total',
and that cosmos.NewUint(common.One)
be 'allocation' (so that the USD units of part and total cancel out, leaving the RUNE units of allocation).
I propose that this comment be changed at the same time: it reads
// the return value is the amount of fee in RUNE
but when looking at the return
lines of the function, the returned values (except when the asset argument is RUNE) are in terms of asset, not RUNE. I thus suggest it read
// the return value is the amount of fee in asset
.