lp: Add compute_at to UpdateTrancheTokenPrice
#1598
Conversation
| @@ -193,6 +193,7 @@ pub mod pallet { | |||
| BalanceRatio = Self::BalanceRatio, | |||
| PoolId = Self::PoolId, | |||
| TrancheId = Self::TrancheId, | |||
| Moment = Seconds, | |||
There was a problem hiding this comment.
I needed to set this restriction here so that we can have the PriceValue.last_updated field bound to Seconds and therefore bound to the computed_at field of UpdateTrancheTokenPrice.
AFAIK this should be an acceptable bound but please let me know if I am overseeing anything here.
There was a problem hiding this comment.
Maybe it can be handled also by creating a MomentOf<T> type and passing to the Message struct.
Having it as Seconds type (which is an u64) could lead to mixing them with other time precisions as milliseconds (also u64). But having it through the associated type is not.
There was a problem hiding this comment.
In the future Seconds and Millis should have wrappers around u64 with math support to avoid mixing them
There was a problem hiding this comment.
But not super strong opinion, I have pallet_loans populated with Seconds right now 😆
There was a problem hiding this comment.
yeah we definitly should have strong typing around those two different units of time 👍
is your suggestion adding a Moment type to the Config of liquidity pools and bound the PriceValue<Moment = T::Moment>? I want to keep the type at the message level concrete and not abstract since messages are type-fixed or else they will break; but we can have a Into<u64> or Into<Seconds> trait bound for this Moment type.
what do you think?
There was a problem hiding this comment.
I want to keep the type at the message level concrete and not abstract since messages are type-fixed or else they will break
But yes, if you want to have more control over the type used, T::moment will work for sure!
There was a problem hiding this comment.
Wondering now, if the encoding dictates everything in this pallet, maybe allowing the user to choose i.e the PoolId or the TrancheId they want is not the safer way.
Should these associated types actually be types?
There was a problem hiding this comment.
yeah exactly, I didn't want to do that because we then need to bound it to u64 to match the expected type in the message. Of course than in a normal scenario that's not what we want so that the pallet is actually generic over these types but for liquidity-pools, a multi-chain system, being type generic over the messages' fields is not desired.
So we can:
-
Keep thing as are with a strict bound on
PriceValue<Moment =which is the least invasive -
Add a
Momenttype to the pallet'Configand havePriceValue<Momentbound to this type AND have the type implementToSecondsorInto<u64>.
I am cool with the second but maybe we could keep things as are now and implement the latter change once we introduce strong typing for Seconds and Millis. wdyt @lemunozm ?
There was a problem hiding this comment.
Yeah, being the message who dictates here, I think leaving this as they're now is ok 👍🏻
As you say, the mixing issue will be fixed with strong typing in Seconds.
After a discussion within the
MultiChainteam, we agreed that we need to include the timestamp at which a tranche token price was calculated in theUpdateTrancheTokenmessage.Given that we add this new field at the end of the message, no migrations or special handing is necessary since this will make this messages backwards and forwards compatible.