-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add StakingPrecompileV2 #1356
base: devnet-ready
Are you sure you want to change the base?
Add StakingPrecompileV2 #1356
Conversation
precompiles/src/staking.rs
Outdated
<<R as frame_system::Config>::Lookup as StaticLookup>::Source: From<R::AccountId>, | ||
{ | ||
const INDEX: u64 = 2053; | ||
const ADDRESS_SS58: [u8; 32] = [0; 32]; |
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.
need set the valid value.
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.
This is not used in the new version, so I set it to 0s (as in Metagraph
and Ed25519Verify
contracts). But I now think, it's better to change this value to Option<[u8; 32]>
in the trait, to prevent potential mistakes.
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.
Done.
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.
Even better would be ridding of this constant and using a conversion from the contract's address. Just look at this value is hard to understand whether it came from the contract address or not. Would be good to have a method in PrecompileExt
trait and use Self::INDEX
to generate this value on-demand. Will introduce a little runtime overhead, but that would be a lot more sound.
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.
looks great.
Description
Recently we changed some methods in
StakingPrecompile
to conform the amount precisions to our Substrate API. They were in ETH-precision before. But that affected some users.This PR takes back ETH-precisions to those methods and introduces a new version of
StakingPrecompile
, that has API consistent with our Substrate API.Tests are here: opentensor/subtensor-js-tests#12
Related Issue(s)
Type of Change
Breaking Change
StakingPrecompile
exists only for backward compatibility from now on and considered deprecated (replaced byStakingPrecompileV2
at a new address)StakingPrecompile::get_stake
now again returns the amount, converted to ETH-precisionStakingPrecompile::remove_stake
accepts amount in ETH-precision as it was beforeChecklist
cargo fmt
andcargo clippy
to 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.