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: Remove portfolio entry when removing a loan + portfolio UT #1561

Merged
merged 1 commit into from
Sep 22, 2023

Conversation

lemunozm
Copy link
Contributor

Description

Optional addition: I think it could be beneficial to leave the portfolio values representing the exact loans found in ActiveLoans. Anyways, the storage is reset for each NAV computation, but having during some time different elements in the portfolio and active loans could lead to errors.

PROS: Data more structured.
CONS: 1 read/write DB for close_loan() action.

Changes and Descriptions

  • add portfolio.remove_elem()
  • add Unitary tests to the portfolio module.

@lemunozm lemunozm added Q1-easy Can be done by primarily duplicating and adapting code. I3-annoyance The code behaves as expected, but "expected" is an issue. D0-ready Pull request can be merged without special precaution and notification. P2-nice-to-have Issue is worth doing. labels Sep 21, 2023
@lemunozm lemunozm self-assigned this Sep 21, 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.

Definitely makes sense!

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.

Reasonable

@lemunozm lemunozm force-pushed the loans/remove_item_portfolio branch from 91cc86b to 9d29e8d Compare September 22, 2023 08:26
@lemunozm lemunozm force-pushed the loans/remove_item_portfolio branch from 9d29e8d to 1ebaf43 Compare September 22, 2023 11:06
@lemunozm lemunozm force-pushed the loans/remove_item_portfolio branch from 1ebaf43 to 6ba6d57 Compare September 22, 2023 12:47
@lemunozm lemunozm merged commit d007468 into main Sep 22, 2023
10 checks passed
@lemunozm lemunozm deleted the loans/remove_item_portfolio branch September 22, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D0-ready Pull request can be merged without special precaution and notification. I3-annoyance The code behaves as expected, but "expected" is an issue. P2-nice-to-have Issue is worth doing. Q1-easy Can be done by primarily duplicating and adapting code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants