Add increase_take and decrease_take extrinsics, rate limiting, and tests by gztensor · Pull Request #380 · opentensor/subtensor

gztensor · 2024-04-29T23:46:58Z

Description

Added:

Extrinsic decrease_take in subtensor pallet
Extrinsic increase_take in subtensor pallet
Extrinsic sudo_set_tx_rate_limit_delegate_take in admin-utils pallet
Separate rate limit logic for increasing delegate take (decreasing is not rate limited)
Unit tests for decreasing and increasing take in misc. corner cases

Related Issue(s)

Closes issue #314

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Other (please describe):

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have run cargo fmt and cargo clippy to ensure my code is formatted and linted correctly
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

orriin · 2024-04-30T03:45:51Z

pallets/subtensor/tests/mock.rs

    pub const InitialStakePruningMin: u16 = 0;
    pub const InitialFoundationDistribution: u64 = 0;
-    pub const InitialDefaultTake: u16 = 11_796; // 18% honest number.
+    pub const InitialDefaultTake: u16 = 32_767; // 50% for tests (18% honest number is used in production (see runtime))


why use a different number in tests and runtime?

yeah I would also prefer consistency here without a good reason not to have it

orriin · 2024-05-01T16:41:26Z

pallets/subtensor/src/staking.rs

+    // ---- The implementation for the extrinsic decrease_take
+    //
+    // # Args:
+    // 	* 'origin': (<T as frame_system::Config>::RuntimeOrigin):
+    // 		- The signature of the caller's coldkey.
+    //
+    // 	* 'hotkey' (T::AccountId):
+    // 		- The hotkey we are delegating (must be owned by the coldkey.)
+    //
+    // 	* 'take' (u16):
+    // 		- The stake proportion that this hotkey takes from delegations for subnet ID.
+    //
+    // # Event:
+    // 	* TakeDecreased;
+    // 		- On successfully setting a decreased take for this hotkey.
+    //
+    // # Raises:
+    // 	* 'NotRegistered':
+    // 		- The hotkey we are delegating is not registered on the network.
+    //
+    // 	* 'NonAssociatedColdKey':
+    // 		- The hotkey we are delegating is not owned by the calling coldket.
+    //
+    pub fn do_decrease_take(
+        origin: T::RuntimeOrigin,
+        hotkey: T::AccountId,
+        take: u16,
+    ) -> dispatch::DispatchResult {
+        // --- 1. We check the coldkey signature.
+        let coldkey = ensure_signed(origin)?;
+        log::info!(
+            "do_decrease_take( origin:{:?} hotkey:{:?}, take:{:?} )",
+            coldkey,
+            hotkey,
+            take
+        );
+
+        // --- 2. Ensure we are delegating a known key.
+        //        Ensure that the coldkey is the owner.
+        Self::do_take_checks(&coldkey, &hotkey)?;
+
+        // --- 3. Ensure we are always strictly decreasing, never increasing take
+        if let Ok(current_take) = Delegates::<T>::try_get(&hotkey) {
+            ensure!(take < current_take, Error::<T>::InvalidTake);
+        }
+
+        // --- 4. Set the new take value.
+        Delegates::<T>::insert(hotkey.clone(), take);
+
+        // --- 5. Emit the take value.
+        log::info!(
+            "TakeDecreased( coldkey:{:?}, hotkey:{:?}, take:{:?} )",
+            coldkey,
+            hotkey,
+            take
+        );
+        Self::deposit_event(Event::TakeDecreased(coldkey, hotkey, take));
+
+        // --- 6. Ok and return.
+        Ok(())
+    }
+
+    // ---- The implementation for the extrinsic increase_take
+    //
+    // # Args:
+    // 	* 'origin': (<T as frame_system::Config>::RuntimeOrigin):
+    // 		- The signature of the caller's coldkey.
+    //
+    // 	* 'hotkey' (T::AccountId):
+    // 		- The hotkey we are delegating (must be owned by the coldkey.)
+    //
+    // 	* 'take' (u16):
+    // 		- The stake proportion that this hotkey takes from delegations for subnet ID.
+    //
+    // # Event:
+    // 	* TakeDecreased;
+    // 		- On successfully setting a decreased take for this hotkey.
+    //
+    // # Raises:
+    // 	* 'NotRegistered':
+    // 		- The hotkey we are delegating is not registered on the network.
+    //
+    // 	* 'NonAssociatedColdKey':
+    // 		- The hotkey we are delegating is not owned by the calling coldket.
+    //
+    // 	* 'TxRateLimitExceeded':
+    // 		- Thrown if key has hit transaction rate limit
+    //
+    pub fn do_increase_take(
+        origin: T::RuntimeOrigin,
+        hotkey: T::AccountId,
+        take: u16,
+    ) -> dispatch::DispatchResult {
+        // --- 1. We check the coldkey signature.
+        let coldkey = ensure_signed(origin)?;
+        log::info!(
+            "do_increase_take( origin:{:?} hotkey:{:?}, take:{:?} )",
+            coldkey,
+            hotkey,
+            take
+        );
+
+        // --- 2. Ensure we are delegating a known key.
+        //        Ensure that the coldkey is the owner.
+        Self::do_take_checks(&coldkey, &hotkey)?;
+
+        // --- 3. Ensure we are strinctly increasing take
+        if let Ok(current_take) = Delegates::<T>::try_get(&hotkey) {
+            ensure!(take > current_take, Error::<T>::InvalidTake);
+        }
+
+        // --- 4. Ensure take is within the 0 ..= InitialDefaultTake (18%) range
+        let max_take = T::InitialDefaultTake::get();
+        ensure!(take <= max_take, Error::<T>::InvalidTake);
+
+        // --- 5. Enforce the rate limit (independently on do_add_stake rate limits)
+        let block: u64 = Self::get_current_block_as_u64();
+        ensure!(
+            !Self::exceeds_tx_delegate_take_rate_limit(
+                Self::get_last_tx_block_delegate_take(&coldkey),
+                block
+            ),
+            Error::<T>::TxRateLimitExceeded
+        );
+
+        // Set last block for rate limiting
+        Self::set_last_tx_block_delegate_take(&coldkey, block);
+
+        // --- 6. Set the new take value.
+        Delegates::<T>::insert(hotkey.clone(), take);
+
+        // --- 7. Emit the take value.
+        log::info!(
+            "TakeIncreased( coldkey:{:?}, hotkey:{:?}, take:{:?} )",
+            coldkey,
+            hotkey,
+            take
+        );
+        Self::deposit_event(Event::TakeIncreased(coldkey, hotkey, take));
+
+        // --- 8. Ok and return.
+        Ok(())
+    }
+


If these both just set the take value, is there a reason they shouldn't be combined into a single extrinsic?

pallets/subtensor/src/staking.rs

orriin

I'm still not convinced we can't combine the increase and decrease extrinsics, but not a big deal and otherwise lgtm

orriin · 2024-05-02T04:00:51Z

pallets/admin-utils/src/lib.rs

@@ -88,7 +88,7 @@ pub mod pallet {
        #[pallet::weight(T::WeightInfo::sudo_set_default_take())]
        pub fn sudo_set_default_take(origin: OriginFor<T>, default_take: u16) -> DispatchResult {


Should this be renamed to reflect the changed functionality?

Suggested change

pub fn sudo_set_default_take(origin: OriginFor<T>, default_take: u16) -> DispatchResult {

pub fn sudo_set_max_delegate_take(origin: OriginFor<T>, max_take: u16) -> DispatchResult {

gregzaitsev added 2 commits April 29, 2024 19:43

Add increase_take and decrease_take extrinsics, rate limiting, and tests

fd1dbe4

Format with cargo fmt

0ef1d2b

gztensor requested review from orriin and sam0x17 April 30, 2024 00:02

orriin approved these changes Apr 30, 2024

View reviewed changes

gregzaitsev and others added 4 commits May 1, 2024 09:11

Add test for setting zero take

5cd3e65

fix test case

a99c55f

Fix the delegate take function in zero delegate take test

3db6b62

Change max take to 18% in tests

19d9078

gztensor requested a review from orriin May 1, 2024 15:24

Fix tests that used 50% take

b733877

orriin reviewed May 1, 2024

View reviewed changes

sam0x17 approved these changes May 1, 2024

View reviewed changes

Add minimum take boundary

4e8d9ca

gztensor requested review from orriin and sam0x17 May 1, 2024 23:53

sam0x17 approved these changes May 2, 2024

View reviewed changes

orriin approved these changes May 2, 2024

View reviewed changes

sam0x17 merged commit c00ff02 into development May 2, 2024

distributedstatemachine deleted the feat/change-delegate-takes-dev branch June 23, 2024 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add increase_take and decrease_take extrinsics, rate limiting, and tests#380

Add increase_take and decrease_take extrinsics, rate limiting, and tests#380
sam0x17 merged 8 commits intodevelopmentfrom
feat/change-delegate-takes-dev

gztensor commented Apr 29, 2024 •

edited

Loading

Uh oh!

orriin Apr 30, 2024

Uh oh!

sam0x17 Apr 30, 2024

Uh oh!

orriin May 1, 2024

Uh oh!

Uh oh!

orriin left a comment

Uh oh!

orriin May 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		@@ -88,7 +88,7 @@ pub mod pallet {
		#[pallet::weight(T::WeightInfo::sudo_set_default_take())]
		pub fn sudo_set_default_take(origin: OriginFor<T>, default_take: u16) -> DispatchResult {

	pub fn sudo_set_default_take(origin: OriginFor<T>, default_take: u16) -> DispatchResult {
	pub fn sudo_set_max_delegate_take(origin: OriginFor<T>, max_take: u16) -> DispatchResult {

Conversation

gztensor commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Type of Change

Checklist

Uh oh!

orriin Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

sam0x17 Apr 30, 2024

Choose a reason for hiding this comment

Uh oh!

orriin May 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

orriin left a comment

Choose a reason for hiding this comment

Uh oh!

orriin May 2, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gztensor commented Apr 29, 2024 •

edited

Loading