Loan manager deposit collateral cei fix#27
Conversation
…they came from current signers
…ounts-approvals-current-signers
…approval invariant
ogazboiz
left a comment
There was a problem hiding this comment.
nice work on this one. the core restructuring is exactly right: you commit the updated collateral to storage before the token transfer and replaced every expect with a typed error, so it now matches the cei pattern used in repay and approve_loan. the malicious-token regression test is a great touch and the assertion (Err(LoanError::LoanNotFound)) lines up with the re-read path.
that said, the pr currently does not compile, so i cannot merge it yet. a few asks:
-
loan_manager/src/lib.rs:1356-let loan: Loanneeds to belet mut loan: Loan. line 1390 mutatesloan.collateral_amount, so the build fails with E0594 (cannot assign, not declared as mutable). -
multisig_governance/src/test.rs:5-6- the//!inner doc comment sits after ausestatement, which rust rejects (E0753). switch those two lines to regular//comments. -
run
cargo fmt- the new import line inloan_manager/src/test.rs:3needs to be split across lines to passcargo fmt --check. -
the multisig_governance changes (count_valid_approvals plus its test) look like they came in from a different branch. could you rebase onto main so this pr only contains the deposit_collateral fix? that keeps it scoped to issue #17 and is what introduces the E0753 above.
one small optional note: the event at lib.rs:1407 still publishes the pre-transfer loan fields rather than the re-read stored_loan. it is harmless since state is already durable before the transfer, just flagging it.
once it compiles and cargo fmt/clippy/test are green, this should be good to go. the logic itself is solid.
if you want to keep contributing, join us on Telegram: https://t.me/+DOylgFv1jyJlNzM0
I added a regression test that installs a malicious token contract which removes the loan during transfer
verifies deposit_collateral returns Err(LoanError::LoanNotFound) safely
Next:
closes #17