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

Swap non vault funds #715

Closed
wants to merge 49 commits into from
Closed

Swap non vault funds #715

wants to merge 49 commits into from

Conversation

magiodev
Copy link
Contributor

@magiodev magiodev commented Jul 26, 2024

1. Overview

As the PR has grown a bit too much, here is a list of the file changes that have been logically impacted, and the ones that have been simply renamed and do not contain any logical changes.

No logical file changes:

  • smart-contracts/osmosis/contracts/cl-vault/src/contract.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/error.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/lib.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/instantiate.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/msg.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/test_helpers.rs
  • smart-contracts/osmosis/contracts/cl-vault/tests/test-tube/any_deposit.rs
  • smart-contracts/osmosis/contracts/cl-vault/tests/test-tube/setup.rs
  • smart-contracts/osmosis/contracts/cl-vault/tests/test-tube/authz.rs

Logical file changes:

  • smart-contracts/osmosis/contracts/cl-vault/src/helpers/getters.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/helpers/msgs.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/vault/deposit.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/vault/range.rs
  • smart-contracts/osmosis/contracts/cl-vault/src/vault/swap.rs
  • smart-contracts/osmosis/contracts/cl-vault/tests/test-tube/autocompound.rs

2. Implementation details

3. How to test/use

4. Checklist

  • Does the Readme need to be updated?

5. Limitations (optional)

6. Future Work (optional)

@magiodev magiodev requested a review from ajansari95 July 26, 2024 10:14
# Conflicts:
#	smart-contracts/osmosis/contracts/cl-vault/src/vault/any_deposit.rs
#	smart-contracts/osmosis/contracts/cl-vault/src/vault/range.rs
#	smart-contracts/osmosis/contracts/cl-vault/src/vault/swap.rs
@magiodev magiodev changed the base branch from fix/swap-non-vault-funds to main July 29, 2024 09:59
@magiodev magiodev changed the title fix swap non vault funds Swap non vault funds Jul 29, 2024
// .checked_add(expected_amount_rewards_to_base.into())
// .unwrap(),
// balances_after_swap_base,
// &deposit_ratio_approx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are few assert_approx i had to comment out temporarily as the deposit_ratio_approx is not dynamic and doesnt allow to approx correctly. Numbers are correct tho. We need to find a way to assert that consistently.

* position_balance.1.to_string().parse::<f64>().unwrap()) as u128,
);

// TODO: Validate that the swap_operation.pool_id_0 is about token_in_denom and pool_config.token0 assets or throw error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validate that the swap_operation.pool_id_base is about token_in.denom we are swapping, and the base token we are swapping into, which in this case should be pool_config.token0 assets, or throw an error. We need to ensure that the twap_price is retrieved in a consistent way before passing it to the calculate_swap_amount alongside the direction of the swap.

.swap_msg,
);

// TODO: Validate that the swap_operation.pool_id_1 is about token_in_denom and pool_config.token1 assets or throw error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Validate that the swap_operation.pool_id_quote is about token_in.denom we are swapping, and the quote token we are swapping into, which in this case should be pool_config.token1 assets, or throw an error. We need to ensure that the twap_price is retrieved in a consistent way before passing it to the calculate_swap_amount alongside the direction of the swap.

&pool_config.token1,
token_in_amount
.checked_multiply_ratio(twap_price.numerator(), twap_price.denominator()),
(twap_price.numerator(), twap_price.denominator()), // TODO: Check if this is correct order for AnyToOne
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As in the previous two comments, we should check that the way we use twap_price here is consistent with how we inject arguments from above. The exact_deposit and any_deposit workflows are hardcoding the right assets on the requests, as we know we always work with asset0 and asset1 (base and quote). This is not the case for the SwapNonVaultFunds, which are accepting arbitrary routes and token ins from outside, and this could be hijackable.

@magiodev
Copy link
Contributor Author

Closing this in favor of #782

@magiodev magiodev closed this Aug 20, 2024
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.

3 participants