-
Notifications
You must be signed in to change notification settings - Fork 247
Type safe swap interface #2011
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
Type safe swap interface #2011
Conversation
a4b5459 to
b3be47d
Compare
b47da7b to
dae1677
Compare
pallets/subtensor/src/tests/mock.rs
Outdated
|
|
||
| let result = result.unwrap(); | ||
|
|
||
| // we don't want to have silent 0 comparissons in tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // we don't want to have silent 0 comparissons in tests | |
| // we don't want to have silent 0 comparisons in tests |
| // let fee = <Test as Config>::SwapInterface::approx_fee_amount(netuid.into(), (amount as f64 * 0.99) as u64); | ||
| // let fee = <Test as Config>::SwapExt::approx_fee_amount(netuid.into(), (amount as f64 * 0.99) as u64); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this suppose to be SwapInterface and not SwapExt ?
| let stake_amount = TaoCurrency::from(100_000_000_000); | ||
|
|
||
| let default_fee = 0; // FIXME: DefaultStakingFee is deprecated | ||
| let default_fee = TaoCurrency::ZERO; // FIXME: DefaultStakingFee is deprecated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment still valid?
| let default_fee = TaoCurrency::ZERO; // FIXME: DefaultStakingFee is deprecated | |
| let default_fee = TaoCurrency::ZERO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test still relies on this value. We should probably refactor the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!
| let order = GetAlphaForTao::<T>::with_amount(u64::MAX); | ||
| let result = T::SwapInterface::swap(netuid.into(), order, limit_price, false, true) | ||
| .map(|r| r.amount_paid_in.saturating_add(r.fee_paid)) | ||
| .map_err(|_| Error::ZeroMaxStakeAmount)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone in the community was complaining about this error. We actually have PriceLimitExceeded, which is much more readable for the end user. I think we can replace it for PriceLimitExceeded because get_max_amount_add is only used for adding or moving stake.
| let order = GetTaoForAlpha::<T>::with_amount(u64::MAX); | ||
| let result = T::SwapInterface::swap(netuid.into(), order, limit_price.into(), false, true) | ||
| .map(|r| r.amount_paid_in.saturating_add(r.fee_paid)) | ||
| .map_err(|_| Error::ZeroMaxStakeAmount)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here: Consider replacing with PriceLimitExceeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll do these changes!
Description
This is a refactoring of swap interface and swap pallet to make it have a static dispatch and to be type safe.
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
cargo fmtandcargo clippyto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.