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

AMM-CDA-4: Integrate Hybrid Router #1277

Closed
wants to merge 23 commits into from

Conversation

Chralt98
Copy link
Member

@Chralt98 Chralt98 commented Mar 1, 2024

What does it do?

What important points should reviewers know?

It isn't required to be compiled. Clippy is also irrelevant. Clippy will be fixed in the final result.

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

References

@Chralt98 Chralt98 added the s:in-progress The pull requests is currently being worked on label Mar 1, 2024
@Chralt98 Chralt98 self-assigned this Mar 1, 2024
@Chralt98 Chralt98 changed the base branch from main to chralt98-amm-cda-release March 1, 2024 17:04
@Chralt98 Chralt98 changed the title AMM-CDA-4 Integrate Hybrid Router AMM-CDA-4: Integrate Hybrid Router Mar 4, 2024
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Mar 5, 2024
@Chralt98 Chralt98 marked this pull request as ready for review March 5, 2024 12:16
@Chralt98 Chralt98 requested a review from sea212 as a code owner March 5, 2024 12:16
@Chralt98 Chralt98 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Mar 6, 2024
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews s:in-progress The pull requests is currently being worked on and removed s:in-progress The pull requests is currently being worked on s:review-needed The pull request requires reviews labels Mar 6, 2024
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on s:review-needed The pull request requires reviews labels Mar 6, 2024
zrml/hybrid-router/README.md Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
@Chralt98 Chralt98 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Mar 13, 2024
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Mar 13, 2024
zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +364 to +380
if !amm_amount_in.is_zero() {
match tx_type {
TxType::Buy => {
T::Amm::buy(&who, market_id, asset, amm_amount_in, BalanceOf::<T>::zero())?;
}
TxType::Sell => {
T::Amm::sell(
&who,
market_id,
asset,
amm_amount_in,
BalanceOf::<T>::zero(),
)?;
}
}
remaining = remaining.checked_sub_res(&amm_amount_in)?;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid there's a bit of a problem here which isn't covered in the ZIP. The buy and sell calls could fail for a variety of reasons. I think we should cover these using more detailed error handling that just question marking the result. In detail:

  • NumericalLimits: Some calculation was not done due to numerical concerns. In this case, you should return Ok(remaining) and basically "continue". This will happen if the pre-execution price is very close to the price of the first order on the book. Basically, the amount_in will be too small for the AMM to handle. Situations like these should not keep the user from executing their trade. We could also build that into the calculate_*_amount_until function, but I think that would just cause more confusion.
  • Everything else is unreachable, provided you check that the market is active before calling this.
Suggested change
if !amm_amount_in.is_zero() {
match tx_type {
TxType::Buy => {
T::Amm::buy(&who, market_id, asset, amm_amount_in, BalanceOf::<T>::zero())?;
}
TxType::Sell => {
T::Amm::sell(
&who,
market_id,
asset,
amm_amount_in,
BalanceOf::<T>::zero(),
)?;
}
}
remaining = remaining.checked_sub_res(&amm_amount_in)?;
}
if !amm_amount_in.is_zero() {
let result = match tx_type {
TxType::Buy => {
T::Amm::buy(&who, market_id, asset, amm_amount_in, BalanceOf::<T>::zero())?;
}
TxType::Sell => {
T::Amm::sell(
&who,
market_id,
asset,
amm_amount_in,
BalanceOf::<T>::zero(),
)?;
}
};
let remaining = match result {
Ok(_) => remaining.checked_sub_res(&amm_amount_in)?,
Err(AmmApiError::SoftFailure) => remaining,
Err(AmmApiError::HardFailure) =>
}
remaining = remaining.checked_sub_res(&amm_amount_in)?;
}

This requires you to add the AmmApiError type to the AmmApi (has two variants, hard and soft failure) and then map the neo-swaps pallet errors to hard and soft failure. Numerical failure is soft in the sense that it could happen during normal operations, whereas everything else is unexpected/"panic".

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, seems legit. This is a bigger suggestion. I will do this as soon as I have looked into the other review comments.

zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +495 to +510
match strategy {
Strategy::ImmediateOrCancel => {
return Err(Error::<T>::CancelStrategyApplied.into());
}
Strategy::LimitOrder => {
T::OrderBook::place_order(
who,
market_id,
maker_asset,
maker_amount,
taker_asset,
taker_amount,
)?;
}
}

Copy link
Member

Choose a reason for hiding this comment

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

We have a similar problem here where a user might end up with a very small remaining amount. If their strategy is LimitOrder, this call will then fail due to the minimum order size set in zrml-orderbook.

zrml/hybrid-router/src/lib.rs Outdated Show resolved Hide resolved
@Chralt98 Chralt98 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Mar 22, 2024
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Mar 22, 2024
@Chralt98
Copy link
Member Author

@maltekliemann and I decided to defer the missing review suggestion to another PR. This decision was made in order to update the APIs needed that are not part of this PR.

@Chralt98
Copy link
Member Author

Changes incorporated here. This PR was not merged as usual, because a normal merge would have caused a revert of some changes.

@Chralt98 Chralt98 closed this Mar 22, 2024
@Chralt98 Chralt98 added s:abandoned This pull request is abandoned and removed s:review-needed The pull request requires reviews labels Mar 22, 2024
@Chralt98 Chralt98 mentioned this pull request Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:abandoned This pull request is abandoned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants