Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loans: ChangeGuard trait and loans modifications #1384

Merged
merged 20 commits into from Jun 7, 2023
Merged

Conversation

lemunozm
Copy link
Contributor

@lemunozm lemunozm commented Jun 5, 2023

Description

This PR overwrites #1222 with some simplifications.

  • The mutation storage is no longer in pallet-loans. Now the implementor of ChangeGuard, is who should allocate those mutations. This way, we extract from pallet-loans and any future ChangeGuard's user the need to manage mutations with its own storage
  • pallet-loans no longer has a dependency on CfgChange. The runtime is who transforms the LoanMutation type into a CfgChange that the implementor of ChangeGuard understand.

Fixes #1207

Changes and Descriptions

  • Added ChangeGuard trait.
  • Added ChangeGuard trait mock.
  • Added 2 new extrinsic for loan modifications to pallet-loans.
  • Added a utility to configure pallet-loans without the modification feature.
  • Legacy tests passing
  • Add new tests
  • Runtimes working with the feature disabled

@lemunozm lemunozm added D0-ready Pull request can be merged without special precaution and notification. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase. P5-soon Issue should be addressed soon. crcl-protocol Circle protocol related. labels Jun 5, 2023
@lemunozm lemunozm self-assigned this Jun 5, 2023
Copy link
Contributor Author

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

Important parts commented below:

pallets/loans-ref/src/lib.rs Show resolved Hide resolved
pallets/loans-ref/src/lib.rs Outdated Show resolved Hide resolved
}
}

pub struct ChangeGuardBridge<Change, ChangeGuardImpl>(PhantomData<(Change, ChangeGuardImpl)>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the most essential part.

Each pallet has its own kind of change. They only know about its change. This ChangeGuardBridge transform that pallet-specific change into a CfgChange that the implementation of ChangeGuard knows.

The current implementation only merges those into one enum. But we could change them into a more detailed one, appending the moment when it happens or adding a severity level that ChangeGuard can understand.

lemunozm

This comment was marked as duplicate.

lemunozm

This comment was marked as duplicate.

hieronx
hieronx previously approved these changes Jun 5, 2023
Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

Clippy is failing but LGTM!

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 5, 2023

I've renamed the main functions of this feature to propose() and apply(), I think these names are more scalable for future applications. Examples:

let change = Change::Loan(loan_id, LoanMutation::InterestRate(...));
let change_id = Loans::propose(origin, pool_id, change)?;
Loans::apply(origin, pool_id, change_id);

But if you do not like or suggest others, please tell me.

@hieronx
Copy link
Contributor

hieronx commented Jun 5, 2023

@lemunozm I do think these extrinsic names are a bit confusing 😅 Since the pallet name is loans, these will be loans.propose() and loans.apply(), which implies you are proposing and applying loans, rather than changes. Maybe just propose_change/mutation and apply_change/mutation?

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 5, 2023

I agree, it makes total sense what you say! 👍🏻

I'll add the suffix _change that I think fits better with the parameter name.

pallets/loans-ref/src/lib.rs Outdated Show resolved Hide resolved
pallets/loans-ref/src/lib.rs Outdated Show resolved Hide resolved
mustermeiszer
mustermeiszer previously approved these changes Jun 6, 2023
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

LGTM! Gives us enough flexibility for future changes and we can channel down core changes to the pool-system and let the rest be handled by the ChangeGuard impl

pallets/loans-ref/src/lib.rs Show resolved Hide resolved
Comment on lines +734 to +741
let (mut loan, _count) = Self::get_active_loan(pool_id, loan_id)?;
transactional::with_transaction(|| {
let result = loan.mutate_with(mutation.clone());

// We do not want to apply the mutation,
// only check if there is no error in applying it
TransactionOutcome::Rollback(result)
})?;
Copy link
Contributor Author

@lemunozm lemunozm Jun 7, 2023

Choose a reason for hiding this comment

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

This is new. It checks that the modification can be applied, at least when it is proposed. It doesn't mean later it can be applied or not, but at least it removes most of the bad usages for it, for example, trying to modify the interest rate in external pricing. If we don't check this, the change will be enqueued, and we will get the error weeks later.

This is not 100% reliable, because once the change can be applied, the loan could have been closed, or its state changed. i.e:

let change_id_1 = Loans::propose_loan_mutation(DCF::DiscountRate(0.2)); // ==> Ok
let change_id_2 = Loans::propose_loan_mutation(ValuationMethod::OutstandingDebt); // ==> Ok

// Time passes and both proposals become available to be applied

Loans::apply_change(change_id_2); // ==> Ok
Loans::apply_change(change_id_1); // ==> MutationError::DiscountedCashFlowExpected

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a great idea! We definitely should incorporate the same in pool-system

@lemunozm
Copy link
Contributor Author

lemunozm commented Jun 7, 2023

This is ready for a final review 🤓

pallets/loans-ref/src/lib.rs Outdated Show resolved Hide resolved
pallets/loans-ref/src/lib.rs Outdated Show resolved Hide resolved
runtime/common/src/lib.rs Show resolved Hide resolved
Copy link
Contributor

@hieronx hieronx left a comment

Choose a reason for hiding this comment

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

LGTM!

@lemunozm lemunozm enabled auto-merge (squash) June 7, 2023 17:20
@lemunozm lemunozm merged commit 3f98dd1 into main Jun 7, 2023
11 checks passed
@lemunozm lemunozm deleted the loans/change-guard branch June 7, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crcl-protocol Circle protocol related. D0-ready Pull request can be merged without special precaution and notification. P5-soon Issue should be addressed soon. Q3-medium Can be done with good experience in the language, but little knowledge of the codebase.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implementation of ChangeGuard trait
3 participants