Refactor: validators #955
Refactor: validators #955
Conversation
f590baf to
3c30985
Compare
3c30985 to
56d5a5a
Compare
39f1099 to
56e8467
Compare
backend/src/db-utils.ts
Outdated
| [ | ||
| `SELECT | ||
| account_id | ||
| const queryNodeValidators = async (): Promise<string[]> => { |
There was a problem hiding this comment.
I suggest we rename this function to queryStakingPoolAccountIds since the current name is misleading
backend/src/index.ts
Outdated
| valueMap: new Map(), | ||
| promisesMap: new Map(), | ||
| }; | ||
| const poolInfos: CachedTimestampMap<ValidatorPoolInfo> = { |
There was a problem hiding this comment.
Let's use "staking pool" terminology since "pool" might be ambiguous (e.g. database connections pool)
backend/src/index.ts
Outdated
| let stakingPoolsInfo = new Map<string, StakingPoolInfo>(); | ||
| let currentStakeInfo = new Map<string, string | undefined>(); | ||
| let stakingPoolsMetadataInfo = new Map<string, PoolMetadataAccountInfo>(); | ||
| const contractBalances: CachedTimestampMap<string> = { |
There was a problem hiding this comment.
How about "staking pool stake proposals"?
There was a problem hiding this comment.
Shouldn't we emphasize that these proposals are somehow different from the proposals we get from RPC (in this case - different by the source)?
Or stakingPoolStakeProposalsFromContract is too much? :)
| for ( | ||
| let currentIndex = 0; | ||
| true; | ||
| currentIndex += VALIDATOR_DESCRIPTION_QUERY_AMOUNT | ||
| ) { |
There was a problem hiding this comment.
nit: while loop would be more readable, I feel
There was a problem hiding this comment.
I actually changed it to regular loop as anything with init and iterating by incrementation is automatically "for loop" for me :)
backend/src/index.ts
Outdated
| } | ||
| }; | ||
|
|
||
| const getValidatorContractBalance = async ( |
There was a problem hiding this comment.
We should use "staking pool" instead of "validator contract" since you can deploy any contract to a validator account, but we actually want to communicate with staking pool contract.
| const getValidatorContractBalance = async ( | |
| const getStakingPoolStakedBalance = async ( |
backend/src/index.ts
Outdated
| const updateContractStakeMap = async ( | ||
| validators: ValidatorEpochData[], | ||
| cachedTimestampMap: CachedTimestampMap<string> | ||
| ): Promise<void> => { | ||
| return updateRegularlyFetchedMap( | ||
| validators | ||
| .filter((validator) => !validator.currentEpoch) | ||
| .map((validator) => validator.accountId), | ||
| cachedTimestampMap, | ||
| getValidatorContractBalance, | ||
| regularFetchStakingPoolsInfoInterval, | ||
| fetchStakingPoolsInfoThrowawayTimeout | ||
| ); | ||
| }; | ||
|
|
||
| const updatePoolInfoMap = async ( | ||
| validators: ValidatorEpochData[], | ||
| cachedTimestampMap: CachedTimestampMap<ValidatorPoolInfo> | ||
| ): Promise<void> => { | ||
| return updateRegularlyFetchedMap( | ||
| validators.map((validator) => validator.accountId), | ||
| cachedTimestampMap, | ||
| getPoolInfo, | ||
| regularFetchStakingPoolsInfoInterval, | ||
| fetchStakingPoolsInfoThrowawayTimeout | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Ideally, it would be great to align the variable names. Currently, they are hard to follow.
cachedTimestampMap name is hard to reason about
There was a problem hiding this comment.
I did my best though some variable names became huge
| const mapValidators = ( | ||
| epochStatus: EpochValidatorInfo, | ||
| poolIds: string[] | ||
| ): ValidatorEpochData[] => { |
There was a problem hiding this comment.
nit: The naming is hard to reason about the actual intent
There was a problem hiding this comment.
Which naming do you mean here? mapValidators? Should it be getValidatorsFromEpochStatus? Or just getValidators?
backend/src/index.ts
Outdated
| }); | ||
| }); | ||
| } | ||
| await updateValidatorDescriptions(stakingPoolsMetadataInfo); |
There was a problem hiding this comment.
nit: the naming of the called function is not aligned with the naming of the outer functions
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Roboto+Mono:wght@500&display=swap" | ||
| rel="stylesheet" | ||
| /> |
There was a problem hiding this comment.
Do we only need it here? Is it some sort of an artifact?
There was a problem hiding this comment.
Actually, we need it one level deeper - in ValidatorTelemetryElements.tsx, as it's the only place that uses that font.
| if (contractStake.gte(seatPriceBN)) { | ||
| return "onHold"; |
There was a problem hiding this comment.
FYI, ideally, on-hold should be only when it is explicitly set on pause in the contract:
There was a problem hiding this comment.
How should we name the one that is not in neither current epoch, nor next epoch or proposal?
| ): StakingStatus | null => { | ||
| if (validator.currentEpoch) { | ||
| if (validator.nextEpoch) { | ||
| return "active"; |
There was a problem hiding this comment.
How do you feel about using SCREAMING_SNAKE_CASE for such const markers? (ACTIVE, ON_HOLD, ...)
There was a problem hiding this comment.
We have a few of types based on string discrimination, some of them are based on PascalCase we get from RPC, the rest is arbitrary.
The only thing I feel uncomfortable is to have a mapping with CAPS keys.
Why did you suggest that? Because of on-hold change to onHold, I presume?
| const nextVisibleStake = | ||
| validator.nextEpoch?.stake ?? validator.afterNextEpoch?.stake; |
There was a problem hiding this comment.
As per our prior discussion we wanted to display the diff to the most recent change of the stake, so when validator submits an intent to unstake, we can clearly see that the stake will drop to 0 soon, so the order should be flipped:
| const nextVisibleStake = | |
| validator.nextEpoch?.stake ?? validator.afterNextEpoch?.stake; | |
| const nextVisibleStake = | |
| validator.afterNextEpoch?.stake ?? validator.nextEpoch?.stake; |
P.S. It deserves a comment in the code with a link to this reply
There was a problem hiding this comment.
So we have a few cases in here, with three stakes that may exist or not: currentEpoch.stake, nextEpoch.stake, afterNextEpoch.stake. We can make a table of what kind of deltas we want to show.
In case proposal exists:
| current | !current | |
|---|---|---|
| next | current -> next | next -> proposal |
| !next | current -> proposal | 0 -> proposal |
In case proposal does not exist:
| current | !current | |
|---|---|---|
| next | current -> next | next -> proposal |
| !next | current -> 0 | - |
Is that what you expect or not?
There was a problem hiding this comment.
These tables look too complex to me, and it seems they are not what I wanted to say.
- Well, my fix was incorrect since I did not rename
nextVisibleStakevariable name. Your logic was correct regarding this naming, but I actually wanted us to display not thenextVisibleStakediff, but rathermostRecentStakediff mostRecentStakeisvalidator.afterNextEpoch?.stake ?? validator.nextEpoch?.stake
There was a problem hiding this comment.
mostRecentStake is validator.afterNextEpoch?.stake ?? validator.nextEpoch?.stake
But in case of an active validator it will be the delta between the current epoch and the proposal, is that actually what we want to do?
There was a problem hiding this comment.
@luixo yes, this is what people expect to see, I believe. In the future we might want to educate people better about the fact that the next epoch validators list is already frozen, and the proposals are landing to the epoch that is the one that is next after the next one, but for now I feel it is fine to just hide that unnecessary technical detail from users.
| // we take "active", "joining", "leaving" validators and sort them firstly | ||
| // after then we sort the rest | ||
| const validatingGroup = ["active", "joining", "leaving"]; | ||
| const sortByBNComparison = (aValue?: string, bValue?: string) => { |
There was a problem hiding this comment.
It seems that new BN(undefined) is BN(0), so it seems we can drop this function altogether and just write:
(a, b) => -new BN(a.currentEpoch?.stake).cmp(new BN(b.currentEpoch?.stake)),
|
https://frontend-pr-955.onrender.com/nodes/validators I am thinking about mixing those "joining" and "proposals" into the list of active nodes but without giving them an index number, and showing their stake in grey font color like it is commonly used to indicate "disabled" state. @shelegdmitriy I feel this used to be the case before, wasn't it? @reefoh Any objections? I don't want us to spend too much time on it, but since we are touching it anyway, we may as well do slight tuning. |
|
On the second thought, I feel we can merge this PR as is, and then consider to have a separate PR addressing the topic I raised in the previous comment. |
I agree with it. From my point of view It's better to focus on such things with new design (for example). Maybe it's better to create an issue and continue this discussion there |
adefa47 to
509998e
Compare
This is a proper refactoring of the most entangled solution in the project - the validators.
Now, we have only two validators types - the one with the data from current epoch (
ValidatorEpochData) and another one extending it with external data - contact info, fee, telemetry etc (ValidatorFullData).Glossary:
get_total_staked_balance, used to fetch balance for potentially-validating-nodes not currently participating in validating process.I'll describe current scheme briefly for historical purposes:
validatorsand remap it to the list of validators with corresponding props.poolv1.nearto the list for a user to pick a newcoming pool with not enough tokens to take a validator seat. For these we get CS.The order of validators rendering is:
We render those validators with amount of tokens:
We render validators token status based on following rules:
activeleavingjoiningproposalonHoldnewcomeridle