Capability to forward tx fees and swap fees to block builder#2426
Capability to forward tx fees and swap fees to block builder#2426sam0x17 merged 10 commits intodevnet-readyfrom
Conversation
| pub struct BlockAuthorFromAura<F>(core::marker::PhantomData<F>); | ||
|
|
||
| impl<F: FindAuthor<u32>> BlockAuthorFromAura<F> { | ||
| pub fn get_block_author() -> Option<AccountId32> { | ||
| let binding = frame_system::Pallet::<Runtime>::digest(); | ||
| let digest_logs = binding.logs(); | ||
| let author_index = F::find_author(digest_logs.iter().filter_map(|d| d.as_pre_runtime()))?; | ||
| let authority_id = pallet_aura::Authorities::<Runtime>::get() | ||
| .get(author_index as usize)? | ||
| .clone(); | ||
| Some(AccountId32::new(authority_id.to_raw_vec().try_into().ok()?)) | ||
| } | ||
| } | ||
|
|
||
| impl AuthorshipInfo<AccountId32> for Runtime { | ||
| fn author() -> Option<AccountId32> { | ||
| BlockAuthorFromAura::<Aura>::get_block_author() | ||
| } | ||
| } | ||
|
|
||
| impl<F: FindAuthor<u32>> AuthorshipInfo<sp_runtime::AccountId32> for BlockAuthorFromAura<F> { | ||
| fn author() -> Option<sp_runtime::AccountId32> { | ||
| Self::get_block_author() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This looks like a reimplementation of FindAccountFromAuthorIndex, no?
If we add the pallet_authorship to the runtime, this becomes a single line in the config, we could have pallet_authorship::Config be a supertrait of pallet_subtensor::Config and access author directly, just adding a pallet helper to derive the AccountId32 from the AuthorityId.
In your tests you would just have to set the pallet_autorship::Author storage value to what you want.
Wdyt?
There was a problem hiding this comment.
Pallet transaction-fee doesn't have config, so we would still have to implement AuthorshipInfo for Runtime. Seems like the trade off is ~15 lines of code vs. including pallet-authorship. IMO this is better, but I'm open to discuss. Let's talk about it in the Nucleus standup.
| pub fee_paid: PaidIn, | ||
| pub fee_to_block_author: PaidIn, |
There was a problem hiding this comment.
Shouldn't these two be the same? Or if I understand correctly you can only pay a part of the fee to the block author and rest goes to the pool?
There was a problem hiding this comment.
Correct: Part of the fee may be paid to the block author.
d1bc5cd
| }); | ||
|
|
||
| // Decrease Alpha outstanding. | ||
| // TODO: Deprecate, not accurate in v3 anymore |
There was a problem hiding this comment.
off topic, but this removed comment is still true.
| // Swap (in a fee-less way) the block builder alpha fee | ||
| let mut fee_outflow = 0_u64; | ||
| let maybe_block_author_coldkey = T::AuthorshipProvider::author(); | ||
| if let Some(block_author_coldkey) = maybe_block_author_coldkey { |
There was a problem hiding this comment.
okay, this took me forever to come to the conclusion that it is correct, but it is correct. I would have taken fees on the claim_fees event that the protocol position calls at each block step because then it's super obivous that accounting is correct, but the accounting here is in fact correct. Just took me a long time to realize it.
| } else { | ||
| // Block author is not found - burn this TAO | ||
| // Pallet balances total issuance was taken care of when balance was withdrawn for this swap | ||
| TotalIssuance::<T>::mutate(|ti| { |
There was a problem hiding this comment.
nit: probably should use the burning utils we have.
|
|
||
| // Increase the balance of the block author | ||
| let maybe_block_author_coldkey = T::AuthorshipProvider::author(); | ||
| if let Some(block_author_coldkey) = maybe_block_author_coldkey { |
There was a problem hiding this comment.
symmetric to alpha case except we don't have to swap back to TAO.
|
|
||
| log::trace!("Delta out: {}", swap_result.delta_out); | ||
| log::trace!("Fees: {}", swap_result.fee_paid); | ||
| log::trace!("Fees for block author: {}", swap_result.fee_to_block_author); |
There was a problem hiding this comment.
lets goo, I believe that Greg and I originally added this long block of trace logs, but I never suspected they would make it into the codebase nor that be expanded upon later.
|
|
||
| // Hold the fees | ||
| Self::add_fees(self.netuid, self.fee); | ||
| // Split fees according to DefaultFeeSplit between liquidity pool and |
There was a problem hiding this comment.
In this section we comment that if we want to take the whole fee we can just do fee_to_block_author = self.fee.
I'm totally okay with this, we just have to make 100% certain that add_fees is called with 0. If it's a positive value we will be double counting fees and create TAO or alpha out of nothing.
| fee_paid: self.fee, | ||
| delta_in: self.delta_in, | ||
| delta_out, | ||
| fee_to_block_author, |
There was a problem hiding this comment.
Just making a note that units are in Alpha or TAO here. Looks good everywhere, but we can't ever assume fee_to_block_author is in TAO terms when working with the SwapStepResult type.
There was a problem hiding this comment.
fee_to_block_author is declared as PaidIn type, so if we're buying, then PaidIn is TaoBalance, and if we're selling, then PaidIn is AlphaBalance. All with type safety.
|
@opentensor/cerebrum / @opentensor/gyrus / @opentensor/cortex breaking change detected! Please prepare accordingly! |
|
marked as breaking change only because some e2es break |
|
Manually tested on a baedeker clone with a transfer. The diff shows:
|
Description
Currently the transaction fees are burned and swap fees go to swap pool. This PR enables:
Also, this PR will raise transaction fees by 10x.
Closes #2424
Type of Change
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctly