modify existing validators() rust api to take both epoch_id and block_id #10097
Conversation
ppca
requested review from
Longarithm,
telezhnaya and
nikurt
and removed request for
Longarithm
ppca
force-pushed
the
xiangyi/rust_api_validators_fix
branch
from
1f0d9bd to
6b1d56e
Compare
core/primitives/src/types.rs
Outdated
| @@ -905,6 +905,8 @@ pub enum EpochReference { | |||
| Latest, | |||
| } | |||
|
|
|||
| pub type MaybeEpochReference = Option<EpochReference>; | |||
There was a problem hiding this comment.
I'm not in favor of defining types for Options. The resulting code is almost the same length, but introduces an additional entity.
ppca
force-pushed
the
xiangyi/rust_api_validators_fix
branch
from
a2e2c66 to
5243988
Compare
chain/jsonrpc/src/api/validator.rs
Outdated
| let block_hash_string = CryptoHash::new().to_string(); | ||
| let params = serde_json::json!([block_hash_string]); | ||
| let result = RpcValidatorRequest::parse(params); | ||
| assert!(result.is_ok()); |
There was a problem hiding this comment.
You don't need this assert, if you do an unwrap() later.
chain/jsonrpc/src/api/validator.rs
Outdated
| epoch_reference: EpochReference::BlockId(BlockId::Hash(block_hash)), | ||
| }; | ||
| let expected_serialized = format!("{expected:?}"); | ||
| assert_eq!(res_serialized, expected_serialized); |
There was a problem hiding this comment.
This works, but the code is much simpler if you define Eq and PartialEq for EpochReference. Then you can simply do assert_eq!(result.unwrap(), RpcValidatorRequest { epoch_reference: EpochReference::BlockId(BlockId::Hash(block_hash)) };
I think it's acceptable because all network messages define Eq and PartialEq. RpcValidatorRequest is almost a network message. :)
chain/jsonrpc/src/api/validator.rs
Outdated
| fn test_serialize_validators_params_as_vec() { | ||
| let block_hash = CryptoHash::new(); | ||
| let block_hash_string = CryptoHash::new().to_string(); | ||
| let params = serde_json::json!([block_hash_string]); |
There was a problem hiding this comment.
Remove block_hash_string and do this instead
| let params = serde_json::json!([block_hash_string]); | |
| let params = serde_json::json!([block_hash.to_string()]); |
chain/jsonrpc/src/api/validator.rs
Outdated
| .try_singleton(|block_id| match block_id { | ||
| Some(id) => Ok(EpochReference::BlockId(id)), | ||
| None => Ok(EpochReference::Latest), | ||
| }) | ||
| .unwrap_or_parse()?; | ||
| Ok(Self { epoch_reference }) | ||
| .unwrap_or_parse() |
There was a problem hiding this comment.
It should be possible to get rid of if-else and have only
Ok(Params::new(value)
.try_singleton(|block_id| match block_id {
Some(id) => Ok(EpochReference::BlockId(id)),
None => Ok(EpochReference::Latest),
})
.unwrap_or_parse())See this as the reference
| #[test] | ||
| fn test_serialize_validators_params_as_object_input_block_height() { | ||
| let block_height: u64 = 12345; | ||
| let params = serde_json::json!({"block_id": block_height}); |
There was a problem hiding this comment.
Very nice, it works the same as we have in blocks API (the ability of parsing both height and hash under the same name)
https://docs.near.org/api/rpc/block-chunk#block-details
Could you please update docs repo as well? Mentioing that we have a new format now
Be careful and don't merge it until we release 1.37
There was a problem hiding this comment.
Hey Olga, what is the docs repo you are referring to?
And when you say don't merge it until we release 1.37, do you mean the change to the docs repo for the api change?
There was a problem hiding this comment.
do you mean the change to the docs repo
yes ofk
It's because the changes in nearcore will be live only after the release of 1.37, while the docs will be updated right after the merge
My PR is also waiting near/docs#1564
There was a problem hiding this comment.
Got it! Will open a PR in docs tmr.
ppca
force-pushed
the
xiangyi/rust_api_validators_fix
branch
from
ce19ca7 to
e326e5e
Compare
validator_by_epoch_id()apivalidators(maybe_block_id)api to bevalidators(maybe_epoch_reference). Basically now within rust, we can inputNone, orSome(EpochReference::EpochId(epoch_id))orSome(EpochReference::BlockId(block_id)).validatorsRPC endpoint are working. When users send requests to the near rpc validators endpoint, they could use as input the legacy format[block_id]or the map format{"epoch_id": epoch_hash}or{"block_id": block hash/height}