-
Notifications
You must be signed in to change notification settings - Fork 322
Handle dust in order_swap.rs and leasing.rs to sync TI #2754
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
base: devnet-ready
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,7 @@ | |
| use super::*; | ||
| use frame_support::{ | ||
| dispatch::RawOrigin, | ||
| traits::{Defensive, fungible::*, tokens::Preservation}, | ||
| traits::{Defensive, fungible::*}, | ||
| }; | ||
| use frame_system::pallet_prelude::OriginFor; | ||
| use frame_system::pallet_prelude::*; | ||
|
|
@@ -92,12 +92,7 @@ impl<T: Config> Pallet<T> { | |
| frame_system::Pallet::<T>::inc_providers(&lease_coldkey); | ||
| frame_system::Pallet::<T>::inc_providers(&lease_hotkey); | ||
|
|
||
| <T as Config>::Currency::transfer( | ||
| &crowdloan.funds_account, | ||
| &lease_coldkey, | ||
| crowdloan.raised, | ||
| Preservation::Expendable, | ||
| )?; | ||
| Self::transfer_tao(&crowdloan.funds_account, &lease_coldkey, crowdloan.raised)?; | ||
|
|
||
| Self::do_register_network( | ||
| RawOrigin::Signed(lease_coldkey.clone()).into(), | ||
|
|
@@ -151,23 +146,13 @@ impl<T: Config> Pallet<T> { | |
| .saturating_mul(U64F64::from(u64::from(leftover_cap))) | ||
| .floor() | ||
| .saturating_to_num::<u64>(); | ||
| <T as Config>::Currency::transfer( | ||
| &lease_coldkey, | ||
| &contributor, | ||
| contributor_refund.into(), | ||
| Preservation::Expendable, | ||
| )?; | ||
| Self::transfer_tao(&lease_coldkey, &contributor, contributor_refund.into())?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] Update leasing weight for contributor refund issuance accounting This still routes each contributor refund through
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] Update leasing weight for contributor refund issuance accounting
|
||
| refunded_cap = refunded_cap.saturating_add(contributor_refund); | ||
| } | ||
|
|
||
| // Refund what's left after refunding the contributors to the beneficiary | ||
| let beneficiary_refund = leftover_cap.saturating_sub(refunded_cap.into()); | ||
| <T as Config>::Currency::transfer( | ||
| &lease_coldkey, | ||
| &who, | ||
| beneficiary_refund, | ||
| Preservation::Expendable, | ||
| )?; | ||
| Self::transfer_tao(&lease_coldkey, &who, beneficiary_refund)?; | ||
|
|
||
| Self::deposit_event(Event::SubnetLeaseCreated { | ||
| beneficiary: who, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -837,22 +837,23 @@ fn test_swap_owner_old_hotkey_not_exist() { | |||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // SKIP_WASM_BUILD=1 RUST_LOG=debug cargo test --test swap_hotkey_with_subnet -- test_swap_owner_new_hotkey_already_exists --exact --nocapture | ||||||||||||||||||||||||||||||||||||||||||||
| // SKIP_WASM_BUILD=1 cargo test --package pallet-subtensor --lib -- tests::swap_hotkey_with_subnet::test_swap_owner_new_hotkey_already_exists --exact --nocapture | ||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||
| fn test_swap_owner_new_hotkey_already_exists() { | ||||||||||||||||||||||||||||||||||||||||||||
| new_test_ext(1).execute_with(|| { | ||||||||||||||||||||||||||||||||||||||||||||
| let old_hotkey = U256::from(1); | ||||||||||||||||||||||||||||||||||||||||||||
| let new_hotkey = U256::from(2); | ||||||||||||||||||||||||||||||||||||||||||||
| let coldkey = U256::from(3); | ||||||||||||||||||||||||||||||||||||||||||||
| let another_coldkey = U256::from(4); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| let netuid = add_dynamic_network(&new_hotkey, &coldkey); | ||||||||||||||||||||||||||||||||||||||||||||
| let netuid = add_dynamic_network(&old_hotkey, &coldkey); | ||||||||||||||||||||||||||||||||||||||||||||
| add_balance_to_coldkey_account(&coldkey, 1_000_000_000_000_u64.into()); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // old_hotkey is owned by coldkey; new_hotkey was already registered on `netuid` | ||||||||||||||||||||||||||||||||||||||||||||
| // by add_dynamic_network (the condition under test). Do NOT reassign new_hotkey to | ||||||||||||||||||||||||||||||||||||||||||||
| // a foreign coldkey — the new_hotkey-ownership check (NonAssociatedColdKey) would | ||||||||||||||||||||||||||||||||||||||||||||
| // then fire before the already-registered-in-subnet check this test targets. | ||||||||||||||||||||||||||||||||||||||||||||
| Owner::<Test>::insert(old_hotkey, coldkey); | ||||||||||||||||||||||||||||||||||||||||||||
| Owner::<Test>::insert(new_hotkey, another_coldkey); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // Perform the swap | ||||||||||||||||||||||||||||||||||||||||||||
| System::set_block_number(System::block_number() + HotkeySwapOnSubnetInterval::get()); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -864,7 +865,7 @@ fn test_swap_owner_new_hotkey_already_exists() { | |||||||||||||||||||||||||||||||||||||||||||
| Some(netuid), | ||||||||||||||||||||||||||||||||||||||||||||
| false | ||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||
| Error::<Test>::HotKeyAlreadyRegisteredInSubNet | ||||||||||||||||||||||||||||||||||||||||||||
| Error::<Test>::NonAssociatedColdKey | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
847
to
+868
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] Restore the already-registered hotkey test case This test is named and documented as covering
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| // Verify the swap | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
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.
[HIGH] Update leasing weight for per-contributor issuance accounting
This refund loop now calls
Self::transfer_tao, which does more storage work than the previous directCurrency::transfer: it checks the source balance, reads Balances total issuance before and after, and can mutate SubtensorTotalIssuancewhen dust is burned. Because this runs once per contributor,register_leased_network(k)gained O(k) additional reads and possible writes, butSubnetLeasingWeightInfo::do_register_leased_networkand the generated benchmark weight were not updated. Re-benchmark/update the weight or manually add the extra DB reads/writes so the extrinsic is not underweighted at highMaxContributors.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.
Bad review. Direct Currency::transfer does not update subtensor pallet TotalIssuance if it burns dust.