Skip to content

Also allow to increase take, add tests#313

Closed
gztensor wants to merge 299 commits intodecrease_takefrom
feature/increase_delegate_take
Closed

Also allow to increase take, add tests#313
gztensor wants to merge 299 commits intodecrease_takefrom
feature/increase_delegate_take

Conversation

@gztensor
Copy link
Contributor

@gztensor gztensor commented Apr 10, 2024

Added:

  • 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

For the code review, please check if this check is needed in do_decrease_take, I removed it because I think it should not be there:

// --- 4. Ensure we are not already a delegate (dont allow changing delegate take.)

This PR closes #314

Copy link
Collaborator

@distributedstatemachine distributedstatemachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The only thing we need to do with is PR is either rebase off development , or merge into stao branch. v1.0.0 has alot of changes and we would need to capture them here. I think it would also be nice to test the delegate take rate limits

Copy link
Collaborator

@distributedstatemachine distributedstatemachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The only thing we need to do with is PR is either rebase off development , or merge into stao branch. v1.0.0 has alot of changes and we would need to capture them here. I think it would also be nice to test the delegate take rate limits

Copy link
Collaborator

@distributedstatemachine distributedstatemachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! The only thing we need to do with is PR is either rebase off development , or merge into stao branch. v1.0.0 has alot of changes and we would need to capture them here. I think it would also be nice to test the delegate take rate limits

I also think it might make sense , so rate limit at the signed extension level , so people dont bombard the chain with these calls

@gztensor
Copy link
Contributor Author

  1. I think the best approach is to create a feature with lowered rate limits and test it in integration tests.
  2. We need to know the transaction is successful in order to enforce the rate limit. Do we know that in chain extensions?

@distributedstatemachine
Copy link
Collaborator

  1. I think the best approach is to create a feature with lowered rate limits and test it in integration tests.

Ok

  1. We need to know the transaction is successful in order to enforce the rate limit. Do we know that in chain extensions?

You can use pre_dispatch hooks on the signed extensions to filter this out . Here is an example of how we use them to filter out some transactions

gregzaitsev and others added 13 commits April 11, 2024 11:35
Co-authored-by: cuteolaf <OliverLim818@gmail.com>
Co-authored-by: cuteolaf <OliverLim818@gmail.com>
Co-authored-by: cuteolaf <OliverLim818@gmail.com>
Co-authored-by: cuteolaf <OliverLim818@gmail.com>
Co-authored-by: cuteolaf <OliverLim818@gmail.com>
Co-authored-by: cuteolaf <OliverLim818@gmail.com>
Co-authored-by: cuteolaf <OliverLim818@gmail.com>
…r/subtensor into feature/increase_delegate_take
@gztensor
Copy link
Contributor Author

@distributedstatemachine , I made changes you requested, and also I rebased the branch to stao (with Polkadot 1.0). Would it make sense to close this PR and open a new one to stao branch instead?

@gztensor
Copy link
Contributor Author

gztensor commented Apr 11, 2024

Also, we could probably close decrease_take branch because this branch incorporates all changes from there.

@distributedstatemachine
Copy link
Collaborator

@distributedstatemachine , I made changes you requested, and also I rebased the branch to stao (with Polkadot 1.0). Would it make sense to close this PR and open a new one to stao branch instead?

Yes this makes sense

Also, we could probably close decrease_take branch because this branch incorporates all changes from there.

Sounds good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants