-
Notifications
You must be signed in to change notification settings - Fork 42
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
Dump validators list from oasisscan and use as fallback #734
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
e2180bd
to
c4b4709
Compare
c4b4709
to
d959a94
Compare
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.
I like this approach. I would perhaps just add a general notification like "Showing validator list as of <DATE-AND-TIME>".
Update: Could we tune the text to not show
Validators
Couldn't load validators. List may be empty or out of date. Error: request https://monitor.oasis.dev/data/validators?limit=500 status 500
but something like:
Validators (last updated: <DATE-AND-TIME>)
Error obtaining validator list from Oasis Monitor: request https://monitor.oasis.dev/data/validators?limit=500 returned status 500.
If everything works, it could just show:
Validators
But that can be done in a follow-up PR.
04e6e34
to
fd6becd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
src/app/state/staking/index.ts
Outdated
|
||
export const initialState: StakingState = { | ||
debondingDelegations: [], | ||
delegations: [], | ||
validators: [], | ||
validators: validatorsDump.validators, |
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.
So with this users can end up delegating to a stale validator that may no longer exist? At a minimum staking operations while this data is being provided to users should be limited to debonding.
NACK.
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.
Fair point, agreed
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.
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.
imo instead of changing initial state we should fallback in saga when a request fails. instead of dispatching updateValidatorsError
action we can dispatch
yield* put(stakingActions.updateValidators(validatorsDump.validators))
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.
One difference in that case (without extra handling): if getting validators first succeeds, but fails second time, result is:
- fallback on error: fallback
- fallback in initial state: first success
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.
ah true, is it possible that between the first and the second request validator could be updated? First success -> validator A works just fine -> something bad happens to validator A -> the second request to monitor fails -> we show fallback in initial state: first success
where validator A is Ok -> user delegates to a stale validator.
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.
Yeah, it is possible
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.
Added a warning before staking to validators with status inactive* or unknown:
* Active/inactive meaning oasisscan/swagger.yml#L306-L309:
If "true", an entity has a node that is registered for being a validator, node is up to date, and has successfully registered itself. However, it may or may not be part of validator set (top <scheduler.params.max_validators> by stake).
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.
While the warning does make things clearer, I am still torn on this because it still forces people that delegate to something not working, into the debonding period. I am still of the opinion that instead of a warning this should just flat out reject the operation.
Inevitably someone will ignore the warning, and later complain once their tokens get stuck in debonding. While it is on them, forcing people to wait till things recover (in what should be an increasingly rare occurence in the future), is likely better.
If people really can't wait to delegate the command line tools will always work...
src/app/state/staking/index.ts
Outdated
@@ -4,11 +4,12 @@ import { useInjectReducer, useInjectSaga } from 'utils/redux-injectors' | |||
|
|||
import { stakingSaga } from './saga' | |||
import { DebondingDelegation, Delegation, StakingState, Validator, ValidatorDetails } from './types' | |||
import * as validatorsDump from 'vendors/smartstake/validators_dump' |
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.
Another issue: this list is inaccurate for testnet
8404629
to
83a293f
Compare
29d2a2c
to
d5c5d40
Compare
e61d268
to
74eac79
Compare
74eac79
to
52932ec
Compare
Part of #751
Part of #749
Currently, oasisscan is down, and oasismonitor is not loading validators.