feat: round result from getSlot RPC method

[lpahlavi]

(XC-292) Round the result from the Solana getSlot RPC method to the nearest n blocks where n is a user parameter given as an input to the canister method. This is done to partly circumvent the fast Solana block time and achieve a higher consensus rate when performing the HTTP outcalls to fetch the latest slot.

[gregorydemay]

Let's have a proper RoundingError struct that wraps a u64. Thanks to minicbor, this comes with little additional overhead in the bytes size. e.g,

[derive(Debug, Decode, Encode)]
pub enum ResponseTransform {
    #[n(0)]
    GetSlot(#[n(1)] RoundingError),
    ...
}
[derive(Debug, Decode, Encode)]
pub struct RoundingError(#[n(0)] u64)

Like this one can also implement Default on RoundingError and remove the constant DEFAULT_ROUNDING_ERROR

[lpahlavi]

Good idea. As discussed, I think this doesn't need to be in the Candid interface (annoying to use for end users since they then have to wrap the value in a new type), however I think this makes sense to use for the client. For this reason I added it to the sol_rpc_types crate including some unit tests. WDYT?

[gregorydemay]

Thanks for adding RoundingError!

however I think this makes sense to use for the client. For this reason I added it to the sol_rpc_type

This part is less clear to me. Why not just have a u64 in the client? I find it a bit weird to have it in sol_rpc_types given that's it's not a Candid type.

[lpahlavi]

I think this could definitely go both ways. My thought process though, is that whereas in the Candid interface we want to simplify things as much as possible to make e.g. CLI commands with dfx easy, with the client it is nice to use types to leverage Rust's typing system in our favor. Here, for example, there is the advantage of having documentation on the RoundingError type with examples etc. IMO this makes it more explicit and better documented what the rounding_error parameter actually does. However, it's true that it is a bit more different from the Candid interface, and maybe it's acceptable to expect developers look there for documentation? WDYT?

[gregorydemay]

I think the current situation is a bit too inconsistent:

1. The Candid interface takes a u64
2. The client takes a dedicated struct RoundingError

I would have public-facing methods (directly calling the SOL RPC canister or using the client) either use u64 or RoundingError but not both. I currently would tend to use a u64:

1. It's usually beneficial for public-facing APIs to be "generous" in the types of parameters they accept (but be restrictive in the return types). This the same argument as the convenience you mentioned for being able to easily use it with dfx.
2. Minimize conversion: now the client takes a RoundingError but needs to convert it back into a u64 to fit it in GetSlotRpcConfig.
3. For the doc, we could point to the doc on the Candid interface.

I'm also ok with both using RoundingError, just not the current mix (happy to discuss online).

[lpahlavi]

Good points. I guess you are right that it makes sense to try and keep a consistent interface between the Candid canister interface and the client interface. I think the Candid interface probably benefits more from the simple u64 type so I removed the RoundingError type from the client method signature and moved it to the canister crate. Let me know if you have any other comments/concerns.

[gregorydemay]

Thanks @lpahlavi for improving the PR! Mostly some minor comments and some understanding question

[gregorydemay]

Thanks for adding RoundingError!

however I think this makes sense to use for the client. For this reason I added it to the sol_rpc_type

This part is less clear to me. Why not just have a u64 in the client? I find it a bit weird to have it in sol_rpc_types given that's it's not a Candid type.

[lpahlavi]

Thanks a lot for the feedback @gregorydemay, LMK what you think of the new changes.

[lpahlavi]

I think this could definitely go both ways. My thought process though, is that whereas in the Candid interface we want to simplify things as much as possible to make e.g. CLI commands with dfx easy, with the client it is nice to use types to leverage Rust's typing system in our favor. Here, for example, there is the advantage of having documentation on the RoundingError type with examples etc. IMO this makes it more explicit and better documented what the rounding_error parameter actually does. However, it's true that it is a bit more different from the Candid interface, and maybe it's acceptable to expect developers look there for documentation? WDYT?

[gregorydemay]

Thanks a lot @lpahlavi for adding the tests. There is the opened question of where to put RoundingError but otherwise LGTM!

[gregorydemay]

I think the current situation is a bit too inconsistent:

1. The Candid interface takes a u64
2. The client takes a dedicated struct RoundingError

I would have public-facing methods (directly calling the SOL RPC canister or using the client) either use u64 or RoundingError but not both. I currently would tend to use a u64:

1. It's usually beneficial for public-facing APIs to be "generous" in the types of parameters they accept (but be restrictive in the return types). This the same argument as the convenience you mentioned for being able to easily use it with dfx.
2. Minimize conversion: now the client takes a RoundingError but needs to convert it back into a u64 to fit it in GetSlotRpcConfig.
3. For the doc, we could point to the doc on the Candid interface.

I'm also ok with both using RoundingError, just not the current mix (happy to discuss online).

[lpahlavi]

@gregorydemay Thanks a lot for the multiple rounds of review and helpful feedback! Let me know if you have any further questions/comments/concerns.

[lpahlavi]

Good points. I guess you are right that it makes sense to try and keep a consistent interface between the Candid canister interface and the client interface. I think the Candid interface probably benefits more from the simple u64 type so I removed the RoundingError type from the client method signature and moved it to the canister crate. Let me know if you have any other comments/concerns.

[gregorydemay]

Thanks a lot for the changes! Would you mind looking for any TODO XC-292 before merging? There should be at least one here