remove subnet owner check#2631
Conversation
open-junius
requested review from
JohnReedV,
evgeny-s,
gztensor,
l0r1s and
sam0x17
| Self::do_burn_alpha(origin, hotkey.clone(), alpha, netuid)?; | ||
|
|
||
| Self::set_rate_limited_last_block(&RateLimitKey::AddStakeBurn(netuid), current_block); | ||
| transactional::with_transaction(|| { |
There was a problem hiding this comment.
I don't think we need with_transaction here, because FRAME should automatically wrap the extrinsic call with the transaction.
There was a problem hiding this comment.
Correct. Every extrinsic is already inside a storage layer so no need for that, failure propagated through ? will rollback the state. All below extrinsic can be simplified too.
There was a problem hiding this comment.
Will also update these extrinsics
There was a problem hiding this comment.
Guys, I see that we call this function do_add_stake_recycle outside the extrinsic call - in the chain extension and since we return Converging, it won't revert the state. We should call Diverging with flag Revert.
It means that we might still need the transaction wrapper.
@open-junius , could you confirm it with the TS e2e test (not Rust, since it won't catch it).
There was a problem hiding this comment.
We can't change the error processing; otherwise, it will change the ABI. So we need to confirm the regression via the test and probably revert the transactional layer here.
cc @l0r1s
There was a problem hiding this comment.
Thanks! just got your point, I will test it in TS e2e. do_add_stake_recycle is defined out of extrinsic, but It will be called in an extrinsic in pallet_contract extrinsic. Anyway, I will verify it.
| // "Add stake and burn" rate limit | ||
| #[codec(index = 6)] | ||
| AddStakeBurn(NetUid), | ||
| AddStakeBurn(NetUid, AccountId), |
There was a problem hiding this comment.
I think it will require a migration of the LastRateLimitedBlock storage item given this is used as the storage key.
There was a problem hiding this comment.
It makes sense, will add a migration function.
open-junius
added
skip-cargo-audit
|
Some Bittensor E2E tests are failing following the change. Maybe @basfroman or @ibraheem-abe can have a look at this? Thanks! |
I can see failed E2E without any code change #2637. There is a new one, sorry, let me check it. The PR remove the subnet owner only check and rate limit restriction https://github.com/opentensor/subtensor/pull/2631/changes#diff-623d1d1d2b9b4882cf7ac464c376245f5efb9a20805d5324e8d384d7afa1d97cL128. Not sure why the E2E is failed. Please @basfroman @ibraheem-abe take a look. |
|
Please ping me if/when this lands so I can update the btcli test. Looks like the failure is due to the rate limit no longer applying. |
|
@thewhaleking It is merged, please update the btcli test. |
Thanks. Updated test in latent-to/btcli#958 |
5 participants
Description
Remove the subnet owner check for do_add_stake_burn
Related Issue(s)
Type of Change
Breaking Change
If this PR introduces a breaking change, please provide a detailed description of the impact and the migration path for existing applications.
Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyScreenshots (if applicable)
Please include any relevant screenshots or GIFs that demonstrate the changes made.
Additional Notes
Please provide any additional information or context that may be helpful for reviewers.