Loans: Moment as Seconds & Millis explicitly #1575

lemunozm · 2023-09-29T13:01:34Z

Description

Instead of having an agnostic Moment type, this PR modifies the codebase to reason about Seconds or Millis and avoid mixing them in pallets.

Changes and Descriptions

New trait when the intention is to obtain the seconds, and another to convert into it.
Added Seconds and Millis types (by now, there are in cfg_traits, but all this machinery should be moved to a future cfg_utils: Internal dependencies organization #1380).
Remove the Moment type, which is a too-open name. The name is still used for traits agnostic of the time unit.
Refactor some pallets to explicitly reason about seconds.
Fixed some time bug found in pallet-loans

Checklist

pallet-loans impl
pallet-loans tests
pallet-interest-accrual impl
pallet-pools impl
pallet-pools tests
permission types => Because internally it retrived seconds, I modify them to use directly the new Seconds type. But I'm not sure if a more correct solution would be to make permission types more independent from secs. Although it would require a refactor to use it from pallet-pool-system correctly I guess. By now, I think using Seconds directly simplifies things.
pallet-liquidity-pools
pallet-registry
update runtime configurations with new time types.

mustermeiszer

Do you in the same run change it for all pallets?

lemunozm · 2023-09-29T14:07:48Z

Until I get exhausted 😆

lemunozm · 2023-10-09T11:46:06Z

pallets/loans/src/entities/loans.rs

+				ActivePricing::External(pricing) => Ok(now
+					>= pricing
+						.last_updated(pool_id)?
+						.into_seconds()
+						.ensure_add(*secs)?),


This was actually a bug. Test were unable to catch this, because we were treating the oracle as seconds too.

Now, since the Moment type coming from the oracles and Seconds from loans are different types we are protected. This kind of issue can be checked now at compile time, forcing you to call into_seconds() to convert among them.

I think is not a severe bug, but not sure if we need to make an upgrade to this.
cc @hieronx @mustermeiszer

What was the bug here? Oracles are actually millies?

Assuming we hit this, this means

now is given in seconds

last_updated is given in milli-seconds

secs is given in seconds

The widest false positive in the Ok(bool) logic we can get will then be at most 0.999 seconds, and as we are not caring for that in the compatrison we will anyways never hit that, right?

Yes, orml_oracle sets the timestamp as milliseconds.

Exactly, so right now, this trigger can not be triggered.

lemunozm · 2023-10-09T13:01:50Z

libs/traits/src/lib.rs

+/// Type to represent milliseconds
+pub type Millis = u64;
+
+/// Type to represent seconds
+pub type Seconds = u64;


An interesting improvement for this would be having them as wrapper types to avoid mixing them in the runtimes. By now, for pallets that use both as pallet-loans, having one by Config solves this issue, forcing to convert them though into_seconds()

lemunozm · 2023-10-11T10:23:10Z

This is finished and can be merged

mustermeiszer

Thanks! That was tedious.

mustermeiszer · 2023-10-16T09:19:53Z

libs/traits/src/lib.rs

+
+impl IntoSeconds for Millis {
+	fn into_seconds(self) -> Seconds {
+		self / 1000


A division by 1000 can never crash

mustermeiszer · 2023-10-16T09:26:33Z

pallets/loans/src/entities/loans.rs

+				ActivePricing::External(pricing) => Ok(now
+					>= pricing
+						.last_updated(pool_id)?
+						.into_seconds()
+						.ensure_add(*secs)?),


What was the bug here? Oracles are actually millies?

mustermeiszer · 2023-10-16T09:35:14Z

pallets/loans/src/entities/loans.rs

+				ActivePricing::External(pricing) => Ok(now
+					>= pricing
+						.last_updated(pool_id)?
+						.into_seconds()
+						.ensure_add(*secs)?),


Assuming we hit this, this means

now is given in seconds

last_updated is given in milli-seconds

secs is given in seconds

The widest false positive in the Ok(bool) logic we can get will then be at most 0.999 seconds, and as we are not caring for that in the compatrison we will anyways never hit that, right?

lemunozm · 2023-10-16T10:22:36Z

Thanks for the review!

As a note aside, related to this PR #1380, probably Seconds / Millis (and maybe even the traits) would be in cfg-utils in the future, because they're generic and runtime-unrelated types

base implementation for loans

8e47ea2

lemunozm requested review from hieronx and mustermeiszer September 29, 2023 13:01

mustermeiszer previously approved these changes Sep 29, 2023

View reviewed changes

implementation for interest-accrual

3892c27

lemunozm dismissed mustermeiszer’s stale review via 3892c27 October 9, 2023 08:54

lemunozm added 2 commits October 9, 2023 13:16

fix pallet-loans

32b40ef

implementation for pool-system & tests

59f7561

lemunozm commented Oct 9, 2023

View reviewed changes

modify pool permission types to use seconds

5efccb7

lemunozm commented Oct 9, 2023

View reviewed changes

lemunozm added 2 commits October 9, 2023 15:03

liquidity-pools updated

5b0aff1

fix errors

c4d2f9b

lemunozm changed the title ~~Loans: moment as secs explicitelly~~ Loans: Moment as Seconds & Millis explicitly Oct 9, 2023

lemunozm requested review from NunoAlexandre, cdamian and wischli October 9, 2023 14:13

lemunozm self-assigned this Oct 9, 2023

lemunozm added I2-bug The code fails to follow expected behaviour. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. labels Oct 9, 2023

lemunozm added 5 commits October 10, 2023 07:55

Merge remote-tracking branch 'origin/main' into loans/moment-as-secs

7186c1d

minor fix

b2b8b45

minor fix

c3925be

fix clippy

a1c4bae

fix clippy

6e8c7f1

lemunozm added 2 commits October 12, 2023 21:51

Merge remote-tracking branch 'origin/main' into loans/moment-as-secs

7dc5fc1

update integration-test generic

c2b2102

lemunozm requested a review from mustermeiszer October 15, 2023 20:49

mustermeiszer approved these changes Oct 16, 2023

View reviewed changes

lemunozm merged commit 470298d into main Oct 16, 2023
10 checks passed

lemunozm deleted the loans/moment-as-secs branch October 16, 2023 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loans: Moment as Seconds & Millis explicitly #1575

Loans: Moment as Seconds & Millis explicitly #1575

lemunozm commented Sep 29, 2023 •

edited

Loading

mustermeiszer left a comment

lemunozm commented Sep 29, 2023

lemunozm Oct 9, 2023

mustermeiszer Oct 16, 2023

mustermeiszer Oct 16, 2023

lemunozm Oct 16, 2023

lemunozm Oct 9, 2023 •

edited

Loading

lemunozm commented Oct 11, 2023

mustermeiszer left a comment

mustermeiszer Oct 16, 2023

lemunozm Oct 16, 2023

mustermeiszer Oct 16, 2023

mustermeiszer Oct 16, 2023

lemunozm commented Oct 16, 2023 •

edited

Loading

Loans: Moment as Seconds & Millis explicitly #1575

Loans: Moment as Seconds & Millis explicitly #1575

Conversation

lemunozm commented Sep 29, 2023 • edited Loading

Description

Changes and Descriptions

Checklist

mustermeiszer left a comment

Choose a reason for hiding this comment

lemunozm commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

lemunozm commented Oct 11, 2023

mustermeiszer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Oct 16, 2023 • edited Loading

lemunozm commented Sep 29, 2023 •

edited

Loading

lemunozm Oct 9, 2023 •

edited

Loading

lemunozm commented Oct 16, 2023 •

edited

Loading