fix: refactor deposit logic #702
Conversation
There was a problem hiding this comment.
I don't quite understand why the rename from any_deposit to deposit.rs. This seems unnecessary and much less consistent. If we want a single deposit.rs file, we should also merge in the logic from exact_deposit.rs, but I tend to prefer keeping things separate. This is just your preference, but in my opinion, it should be one or the other.
There was a problem hiding this comment.
After checking out the branch locally, I realized that you did, hehe. The file changes on GitHub are not showing any change in the exact_deposit.rs file. Anyway, I still think that merging those two into one becomes monolithic and unnecessary. The execute_deposit() function is very large, and having a big match there with nested logic, or even wrapped by helper functions, doesn’t provide any clean proposition nor enhance the readability.
ajansari95
force-pushed
the
fix/deposit-refactor
branch
from
6e5f871 to
d518490
Compare
There was a problem hiding this comment.
The addition of the Deposit type enum does not make a whole lot of sense to me since that same information is already implied by the two different entry points. For this PR, I'd remove any changes related to that enum's addition and discuss that in slack and restrict the PR to the math refactor in range.rs
| @@ -173,6 +173,11 @@ pub struct InstantiateMsg { | |||
| pub initial_upper_tick: i64, | |||
| } | |||
|
|
|||
| pub enum DepositType { | |||
There was a problem hiding this comment.
Is this distinction not already made by the two different entrypoints?
|
|
||
| let user_value = get_asset0_value(deps.storage, &deps.querier, deposit.0, deposit.1)?; | ||
| let refund_value = get_asset0_value(deps.storage, &deps.querier, refund.0, refund.1)?; | ||
| // NOTE: both the deposit arms here have some repeated math logic, |
There was a problem hiding this comment.
If we combine both entrypoints into a single function call, why do we still have math logic that is repeated?
1. Overview
2. Implementation details
3. How to test/use
4. Checklist
5. Limitations (optional)
6. Future Work (optional)