-
Notifications
You must be signed in to change notification settings - Fork 157
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
Use alpha for stake in neuron info #888
Conversation
The reason we use |
But in Or is it |
Can we add additional field |
5dcb332
to
9952d4e
Compare
I believe this change is attempting to keep the same struct type-wise, while changing the value to be as meaningful to the client as it was. When we didn't have the |
We could, but the big issue with this is we don't have proper versioning set up for the RuntimeAPIs and RPCs. So this will break any clients that rely on this API (many). |
@camfairchild , we have |
Closing, as To fix the inconsistency between |
Description
This PR fixes inconsistency issues related to staking values returned by RPC's
NeuronInfoRuntimeApi
andStakeInfoRuntimeApi
.Getters for the
stake
values are added toNeuronInfo
,NeuronInfoLite
andStakeInfo
, because there is no other way to test this values in the integration tests.Related Issue(s)
Type of Change
Checklist
cargo fmt
andcargo clippy
to ensure my code is formatted and linted correctlyAdditional Notes
It would be better to introduce different types for TAO and Alpha balances, so these kinds of errors would be discoverable at the compilation time (#887).
Also, to prevent possible inconsistencies between
NeuronInfo
andNeuronInfoLite
it would be better to makeNeuronInfoLite
a part ofNeuronInfo
, instead of duplicating information between them.Allowing unit/module tests would help with private API testing. In case the module tests are not possible, another option could be a cargo feature, which would make API dedicated to tests being compiled for tests only (
#[cfg(test)]
doesn't work for the integration tests).