feat: unified trade events #910
Conversation
|
what if we call the new pallet something more generic ... eg. amm-support. it might be useful for other things, not only one specific trade event. |
|
New crates:
Crate versions that have been updated:
Runtime version has been increased. |
pallets/omnipool/src/lib.rs
Outdated
| let additional_fees_total: Balance = fees | ||
| .clone() | ||
| .into_iter() | ||
| .filter_map(|opt| opt.map(|(balance, _)| balance)) |
There was a problem hiding this comment.
since you filter out zero amounts later on, you can do it directly here
| .filter_map(|opt| opt.map(|(balance, _)| balance)) | |
| .filter_map(|opt| opt.map(|(balance, _)| if balance.is_zero(){ | |
| None | |
| } | |
| else{ | |
| Some(balance) | |
| })) |
something like that should work
There was a problem hiding this comment.
but that should not really return 0 fee amounts anyway.
There was a problem hiding this comment.
i see your point, but here I sum the total, instead of converting them to some entries, so having the current implementation makes more sense imo.
As for the zero amount, it can only happen if we confugre 0% in runtime, possibly no chance, but since it is still possible, I wanna protect from it.
pallets/omnipool/src/lib.rs
Outdated
|
|
||
| let additional_fee_entries: Vec<Fee<T::AccountId>> = fees | ||
| .iter() | ||
| .filter_map(|opt| { |
There was a problem hiding this comment.
no need for filter map, just map
There was a problem hiding this comment.
since tkaen fee can contain optionals, and I wanna filter them out, filter map seems like the better option
pallets/omnipool/src/lib.rs
Outdated
| }]; | ||
|
|
||
| let additional_fee_entries: Vec<Fee<T::AccountId>> = fees | ||
| .iter() |
There was a problem hiding this comment.
no need to iter over references here. you can directly consume the fees. so you avoid clones and stuff.
but that requires my previous comment to filter out zeors previously.
There was a problem hiding this comment.
ok I refactored it, containing less filtering, not exactly how you proposed but similar
Fixes #755
Additional todos:
TODO: