Skip to content

Loan manager deposit collateral cei fix#27

Open
jotel-dev wants to merge 4 commits into
LabsCrypt:mainfrom
jotel-dev:loan-manager-deposit-collateral-cei-fix
Open

Loan manager deposit collateral cei fix#27
jotel-dev wants to merge 4 commits into
LabsCrypt:mainfrom
jotel-dev:loan-manager-deposit-collateral-cei-fix

Conversation

@jotel-dev

@jotel-dev jotel-dev commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

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

@ogazboiz ogazboiz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

  1. loan_manager/src/lib.rs:1356 - let loan: Loan needs to be let mut loan: Loan. line 1390 mutates loan.collateral_amount, so the build fails with E0594 (cannot assign, not declared as mutable).

  2. multisig_governance/src/test.rs:5-6 - the //! inner doc comment sits after a use statement, which rust rejects (E0753). switch those two lines to regular // comments.

  3. run cargo fmt - the new import line in loan_manager/src/test.rs:3 needs to be split across lines to pass cargo fmt --check.

  4. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Contracts] LoanManager deposit_collateral re-reads the loan after a token transfer, creating a check-then-act gap

2 participants