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

disable trade route ICA swap #1137

Merged
merged 2 commits into from
Mar 19, 2024
Merged

disable trade route ICA swap #1137

merged 2 commits into from
Mar 19, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Mar 15, 2024

Small one here but definitely double check me that this is all we have to do to disable the swaps.

@sampocs sampocs changed the title disable swap disable trade route ICA swap Mar 15, 2024
@ethan-stride
Copy link
Contributor

This will indeed stop the chain from doing swaps and I do think it is all that is needed. The 3 phase design we ended on means the swaps were their own phase and their callbacks don't exist. The third phase is still the return which exists in the TransferAllRewardTokens keeper function.

But you could change the swap route types and all the associated tx routes to update trade routes if we are sure we won't want to go back since now they no longer need info for how to do the swaps like which pool and slippage params etc. If we think we might want to enable them again, then you can leave them be.

Copy link
Contributor

@ethan-stride ethan-stride left a comment

Choose a reason for hiding this comment

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

LGTM

@ethan-stride
Copy link
Contributor

Side note: because the return path does an ICQ only looking for DYDX tokens in the TradeAccount ICA it would ignore other tokens in there... so the operator could actually do multiple swaps if they thought it would give a better price. Eg. Swap USDC for OSMO then swap the OSMO for DYDX

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

should we remove the price query too? the authz grantee could just query the price offchain right before swapping rather than using on-chain metric that is more stale

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

If you're confident we don't make assumptions around the price being non-nil, I'm signed off

@sampocs sampocs merged commit 1e04151 into rebate Mar 19, 2024
1 check passed
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.

5 participants