[GraphQL] Fix APY for a validator #17701
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@stefan-mysten is attempting to deploy a commit to the Mysten Labs Team on Vercel. A member of the Team first needs to authorize it. |
From my experience, calculating the APYs has always been slow at the beginning of the epoch due to all the dynamic field loading and calculation but it's only the case for the first time of the epoch and will be cached later, so we were okay with this slowness. |
You're absolutely right. It takes roughly ~40-50s for the first query to finish, so it times out. This is not ideal, so I think the plan is that from GraphQL side we'll kick the |
stefan-mysten
force-pushed
the
gql_fix_apy
branch
from
55ecf87
to
58582b6
Compare
stefan-mysten
force-pushed
the
gql_fix_apy
branch
from
58582b6
to
85b7955
Compare
stefan-mysten
force-pushed
the
gql_fix_apy
branch
from
85b7955
to
0be7e05
Compare
There was a problem hiding this comment.
I was initially confused because I thought when we evict the cache, we only fetch exchange rates info for the new latest epoch, but iiuc we're actually fetching the entire exchange_rates_size number of PoolTokenExchangeRate for each validator, which should correspond to the entire history of epochs in which they have rates for (something like that)
With that understanding I think I follow why we can just have the task make an exchange_rates call on each epoch change. Although I think it's not ideal to go back and forth on the implementation in governance_api, and we might be better off even just copying the code as is into graphql so we can further transform it into something that better integrates with our code (using dataloader, etc.)
And other things like using a dynamic fields dataloader to load the latest sui system state + validator dynamic fields, fetching historical sui system state off an epoch using graphql implementation of epoch query, etc.
But otherwise I think this checks out
| let apys = calculate_apys( | ||
| stake_subsidy_start_epoch, | ||
| vec![validator_exchange_rates_to_use], | ||
| ); | ||
| Ok(apys.iter().find(|x| x.address == *address).map(|x| x.apy)) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } | ||
|
|
||
| governance_api | ||
| .get_validator_apy(address) | ||
| pub(crate) async fn fetch_exchange_rates( | ||
| &self, | ||
| system_state: &NativeSuiSystemStateSummary, | ||
| ) -> Result<Vec<ValidatorExchangeRates>, Error> { | ||
| let governance_api = GovernanceReadApi::new(self.inner.clone()); | ||
| exchange_rates(&governance_api, system_state) | ||
| .await | ||
| .map_err(|e| Error::Internal(format!("{e}"))) | ||
| .map_err(|e| Error::Internal(format!("Error fetching exchange rates. {e}"))) |
There was a problem hiding this comment.
was hoping that we could reimplement these through pg.rs instead of relying on the existing jsonrpc implementation, firstly because it looks like we need to modify json-rpc implementation, and secondly so we can set things up to plug into dataloader
There was a problem hiding this comment.
Just a note, the only thing we're relying on from JSON RPC is ValidatorExchangeRates and calculate_apys. Ultimately, we'll have to port those here, but it was easier to do it as this to keep things consistent.
There was a problem hiding this comment.
I am not sure how to do what you suggested. The exchange_rates is quite involved, doing a lot of dynamic fields queries...
I switched to a data loader but still using the exchange_rates in the back as a cache.
| let latest_sui_sytem_state = self.db.inner.spawn_blocking(move |this| | ||
| this.get_latest_sui_system_state() | ||
| ).await.map_err(|_| error!("Failed to fetch latest Sui system state")); |
There was a problem hiding this comment.
It would be nice to route this through graphql db implementation eventually, and/ or we can implement ObjectStore for db?
There was a problem hiding this comment.
I think the issue here is that the ServerBuilder type only has a Db which is a PgExecutor. But agreed, perhaps we should expose PgManager to the ServerBuilder and get the PgExecutor as needed.
| @@ -268,7 +272,11 @@ impl Validator { | |||
| async fn apy(&self, ctx: &Context<'_>) -> Result<Option<u64>, Error> { | |||
| Ok(ctx | |||
| .data_unchecked::<PgManager>() | |||
| .fetch_validator_apys(&self.validator_summary.sui_address) | |||
| .fetch_validator_apys( | |||
| &self.latest_sui_system_state, | |||
There was a problem hiding this comment.
I suppose we're doing this so as to not incur the need to fetch latest_sui_system_state n times for the number of validators. Alternatively, maybe we can use the dynamic fields dataloader to fetch the latest sui system state, so we don't need to thread this through?
There was a problem hiding this comment.
Correct, really not ideal - there's also a .clone() somewhere if my memory serves me well, so yes, a dataloader here should be much better.
There was a problem hiding this comment.
Switched to a data loader that returns the Vec<(EpochId, PoolTokenExchangeRate)>which can be passed tocalculate_apys. Also moved into this crate the calculate_apys` function.
Move exchange_rates to be a free function, and remove unneeded APIs. GraphQL APY code is reworked in #17701. This PR needs to be merged first, before #17701. Note that the diff messes things up. The code was not changed, except for `self -> state`. ## Description Describe the changes or additions included in this PR. ## Test plan How did you test the new or updated feature? --- ## Release notes Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required. For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK:
stefan-mysten
force-pushed
the
gql_fix_apy
branch
from
06af7f9
to
4607525
Compare
|
I will merge this now, and then revisit it once we decide what to do with this API. |
Description
This PR fixes the APY for a validator and ensures that historical APYs can be computed. It also reverts some changes in the
governance_api, particularly movesexchange_ratesas a free function that can be called from the GraphQL crate.At system start, the task to trigger the exchange rates cache is executed, thus until the next trigger on epoch change, the apys can be fetched pretty fast (~2-3s for the standard 5 items per page).
Test plan
Updated test. Manual tests and queries.
Response time: ~1s
{ epoch { epochId referenceGasPrice validatorSet { totalStake inactivePoolsSize activeValidators(first: 5) { nodes { name apy } } } } }{ "data": { "epoch": { "epochId": 405, "referenceGasPrice": "751", "validatorSet": { "totalStake": "8163113083622628156", "inactivePoolsSize": 4, "activeValidators": { "nodes": [ { "name": "ComingChat", "apy": 310 }, { "name": "InfStones", "apy": 302 }, { "name": "Gaucho", "apy": 310 }, { "name": "Astro-Stakers", "apy": 310 }, { "name": "H2O Nodes", "apy": 310 } ] } } } } }Response time for this query: ~2s
{ epoch(id: 127){ epochId referenceGasPrice validatorSet { totalStake inactivePoolsSize activeValidators { nodes { name apy } } } } }{ "data": { "epoch": { "epochId": 127, "referenceGasPrice": "765", "validatorSet": { "totalStake": "7437800674887300328", "inactivePoolsSize": 2, "activeValidators": { "nodes": [ { "name": "ComingChat", "apy": 480 }, { "name": "InfStones", "apy": 471 }, { "name": "Gaucho", "apy": 471 }, { "name": "Astro-Stakers", "apy": 485 }, { "name": "H2O Nodes", "apy": 471 } ] } } } } }Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.