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.

Sign up for GitHub

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

Jump to bottom

Remove duplicate dex_router state #803

Merged
merged 2 commits into from
Aug 30, 2024
Merged

Remove duplicate dex_router state #803

merged 2 commits into from
Aug 30, 2024

Conversation

lubkoll
Copy link
Contributor

@lubkoll lubkoll commented Aug 21, 2024

No description provided.

arhamchordia
arhamchordia previously approved these changes Aug 21, 2024
View reviewed changes
Copy link
Collaborator

@arhamchordia arhamchordia left a comment

Choose a reason for hiding this comment

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

Went through it. Looks good.
Can merge once there is another review.

@lubkoll lubkoll force-pushed the feat/rm-dex-router branch 10 times, most recently from 6ae0150 to 88dfbc4 Compare August 21, 2024 17:51
@0xLaurenzo 0xLaurenzo self-requested a review August 22, 2024 19:09
0xLaurenzo
0xLaurenzo previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@0xLaurenzo 0xLaurenzo left a comment

Choose a reason for hiding this comment

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

lgtm

smart-contracts/osmosis/contracts/cl-vault/src/vault/deposit.rs
@@ -57,7 +56,7 @@ pub(crate) fn execute_any_deposit(
}

let twap_price = get_twap_price(deps.storage, &deps.querier, &env, 24u64)?;
let (token_in, out_denom, remainder, price) = if !deposit_info.base_refund.amount.is_zero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why does a pr about removing dex router change remainder here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also not sure why we are changing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see long explanation below

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense now, to add to the confusion github's view cut this weirdly to make me think this was part of exact_deposit 😓

@0xLaurenzo 0xLaurenzo dismissed their stale review August 22, 2024 19:20

i am braindead sometimes

@0xLaurenzo 0xLaurenzo self-requested a review August 22, 2024 19:21
@lubkoll lubkoll force-pushed the feat/rm-dex-router branch from 88dfbc4 to db77ba0 Compare August 22, 2024 19:21
0xLaurenzo
0xLaurenzo reviewed Aug 22, 2024
View reviewed changes
Copy link
Contributor

@0xLaurenzo 0xLaurenzo left a comment

Choose a reason for hiding this comment

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

Not sure about why we have certain changes to deposit.rs other than that lgtm and ready to merge once explained or sorted

@lubkoll
Copy link
Contributor Author

lubkoll commented Aug 22, 2024 •
edited
Loading

Not sure about why we have certain changes to deposit.rs other than that lgtm and ready to merge once explained or sorted

:D yes, sry for the confusion, probably should have left a comment of all the things I changed to make this actually work as that was puzzling for me too. So here is roughly why the unexpected changes are here.

  • in the test-tube tests we were mainly assuming that the dex_router is not set and were doing direct swaps against osmosis
  • when handling the response from AnyDeposit, then this was also only tested directly against osmosis, and incorrectly assuming that we get a direct response from osmosis to read the received tokens (i.e. we did a cast of the result to a response from osmosis). This of course failed when using the dex_router. So I removed that and instead just used the balances from the contract to establish a new position. As a consequence all the data stored in CURRENT_SWAP_ANY_DEPOSIT were not used anymore, except the recipient, so I removed these too.
  • as by now you may have guessed all the changes of the values in the tests are also due to swaps through the dex_router instead of direct swaps against osmosis

tldr: test-tube setup was bad and missing a bug, so had to fix a lot to make things actually work

can do a first fix of the current code and then rebase this so it just contains the removal of the DEX_ROUTER tomorrow

0xLaurenzo
0xLaurenzo previously approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@0xLaurenzo 0xLaurenzo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

thanks, makes sense and lgtm!

smart-contracts/osmosis/contracts/cl-vault/src/vault/deposit.rs
@@ -57,7 +56,7 @@ pub(crate) fn execute_any_deposit(
}

let twap_price = get_twap_price(deps.storage, &deps.querier, &env, 24u64)?;
let (token_in, out_denom, remainder, price) = if !deposit_info.base_refund.amount.is_zero() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense now, to add to the confusion github's view cut this weirdly to make me think this was part of exact_deposit 😓

@lubkoll
Copy link
Contributor Author

lubkoll commented Aug 22, 2024

I am afraid that this still is not good enough. It will pick up idle funds incorrectly and attribute it to the current deposit. So need to fix that first.

@lubkoll lubkoll changed the title Remove duplicate dex_router state WIP: Remove duplicate dex_router state Aug 22, 2024
magiodev
magiodev suggested changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@magiodev magiodev left a comment

Choose a reason for hiding this comment

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

dmed

@lubkoll lubkoll force-pushed the feat/rm-dex-router branch from db77ba0 to d989136 Compare August 28, 2024 09:17
@lubkoll lubkoll changed the base branch from main to feat/test-helper August 28, 2024 09:17
@lubkoll lubkoll changed the title WIP: Remove duplicate dex_router state Remove duplicate dex_router state Aug 28, 2024
Base automatically changed from feat/test-helper to main August 28, 2024 09:35
@lubkoll lubkoll force-pushed the feat/rm-dex-router branch from d989136 to ed68c61 Compare August 28, 2024 09:36
ajansari95
ajansari95 approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@ajansari95 ajansari95 left a comment

Choose a reason for hiding this comment

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

lgtm

0xLaurenzo
0xLaurenzo approved these changes Aug 29, 2024
View reviewed changes
Copy link
Contributor

@0xLaurenzo 0xLaurenzo left a comment

Choose a reason for hiding this comment

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

lgtm

magiodev
magiodev approved these changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@magiodev magiodev left a comment

Choose a reason for hiding this comment

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

looks good now after the adjustments

@lubkoll lubkoll merged commit d75209f into main Aug 30, 2024
3 checks passed
@lubkoll lubkoll deleted the feat/rm-dex-router branch August 30, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajansari95 ajansari95 ajansari95 approved these changes

@0xLaurenzo 0xLaurenzo 0xLaurenzo approved these changes

@magiodev magiodev magiodev approved these changes

@arhamchordia arhamchordia Awaiting requested review from arhamchordia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lubkoll @magiodev @0xLaurenzo @arhamchordia @ajansari95