Conversation
ales-otf
added
skip-cargo-audit
shamil-gadelshin
left a comment
shamil-gadelshin
left a comment
There was a problem hiding this comment.
Looks good. Just one question about SwapInterface.
ales-otf
requested review from
JohnReedV,
camfairchild,
gztensor,
l0r1s,
liamaharon,
open-junius,
sam0x17,
shamil-gadelshin and
unconst
open-junius
left a comment
open-junius
left a comment
There was a problem hiding this comment.
Looks good to me. It is very helpful to understand the code. and make code more like typed Rust. Thanks!
l0r1s
left a comment
l0r1s
left a comment
There was a problem hiding this comment.
It gives a lot more type safety and document the code tremendously, well done!
ales-otf
dismissed stale reviews from gztensor, l0r1s, open-junius, and shamil-gadelshin
via
282f481
ales-otf
force-pushed
the
sasha/chore/currency-types
branch
from
282f481 to
608108d
Compare
| .saturating_to_num::<u64>(); | ||
| return Self::get_max_amount_add(destination_netuid, destination_subnet_price) | ||
| .map(Into::into); | ||
| // FIXME: mixed types alpha/tao |
There was a problem hiding this comment.
in different places get_max_amount_add is used both: for calculating tao and alpha. it needs to be refactored, but it's outside of the scope of a wrapper for Tao/Alpha. i just highlighted this issue. the code is not changed.
| .saturating_to_num::<u64>() | ||
| .into(); |
There was a problem hiding this comment.
Could be cool to impl From for these floating point types so we can skip the intermediary u64 step. not a blocker
| // The initial TAO is the locked amount, with a minimum of 1 RAO and a cap of 100 TAO. | ||
| let pool_initial_tao = Self::get_network_min_lock(); | ||
| let pool_initial_alpha = AlphaCurrency::from(Self::get_network_min_lock()); | ||
| // FIXME: the result from function is used as a mixed type alpha/tao |
| _, | ||
| Identity, | ||
| NetUid, | ||
| TaoCurrency, |
There was a problem hiding this comment.
I don't think we need one, but should double check don't need a migration for these
There was a problem hiding this comment.
Per the code this storage holds Tao
| /// --- MAP ( key ) --> last_tx_block_delegate_take | ||
| pub type LastTxBlockDelegateTake<T: Config> = | ||
| StorageMap<_, Identity, T::AccountId, u64, ValueQuery, DefaultLastTxBlock<T>>; | ||
| // FIXME: this storage is used interchangably for alpha/tao |
There was a problem hiding this comment.
this is pretty clear again, as I said above
| // FIXME this function is used both to calculate for alpha stake amount as well as tao | ||
| // amount |
| // We are using the sp_arithmetic sqrt here, which works for u128 | ||
| let liquidity = helpers_128bit::sqrt( | ||
| (tao_reserve as u128).saturating_mul(u64::from(alpha_reserve) as u128), | ||
| (tao_reserve.to_u64() as u128).saturating_mul(alpha_reserve.to_u64() as u128), |
There was a problem hiding this comment.
probably makes sense to impl From<TaoCurrency> for u128?
| fn to_u64(&self) -> u64 { | ||
| (*self).into() | ||
| } |
There was a problem hiding this comment.
curious what the purpose of this wrapper is, instead of using into
There was a problem hiding this comment.
in some cases compiler can not know exact type in compilation time, so you can't use into, and then you should do something like Into::<u64>::into(value) or u64::from(value). here because the function returns u64, compiler knows into which type to convert.
| position_id, | ||
| liquidity: liquidity_delta, | ||
| tao: (result.tao as i64).neg(), | ||
| tao: (result.tao.to_u64() as i64).neg(), |
There was a problem hiding this comment.
I don't think result.tao.to_u64() as i64 is safe, it will wrap for large numbers
There was a problem hiding this comment.
my change doesn't introduce any changes in this context. result.tao previously was u64 already. my change just converts TaoCurrency into u64 as it was before.
| position_id, | ||
| liquidity: liquidity_delta, | ||
| tao: result.tao as i64, | ||
| tao: result.tao.to_u64() as i64, |
There was a problem hiding this comment.
I don't think result.tao.to_u64() as i64 is safe, it will wrap for large numbers
There was a problem hiding this comment.
the same here as I said before
liamaharon
left a comment
liamaharon
left a comment
There was a problem hiding this comment.
Looks good! it would be great to open some issues for the final todos
7 participants
Description
This is the second part of this PR.
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.