Add slot to the error context of EpochRewardsPeriodActiveErrorData and SlotNotEpochBoundaryErrorData

[steveluscher]

Problem

Without slot on the error context here, clients can't localize this message. That is to say they can't convert the error code -32017 or -32018 to a localized message that includes the value of slot by combining the context data with a custom message.

Summary of Changes

1. Add slot to EpochRewardsPeriodActiveErrorData
2. Add SlotNotEpochBoundaryErrorData with a slot property

[mergify]

If this PR represents a change to the public RPC API:

1. Make sure it includes a complementary update to rpc-client/ (example)
2. Open a follow-up PR to update the JavaScript client @solana/kit (example)

Thank you for keeping the RPC clients in sync with the server API @steveluscher.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.2%. Comparing base (8adcd5a) to head (3c96902).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #6962     +/-   ##
=========================================
- Coverage    83.2%    83.2%   -0.1%     
=========================================
  Files         853      853             
  Lines      374698   374726     +28     
=========================================
+ Hits       311800   311819     +19     
- Misses      62898    62907      +9     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[buffalojoec]

The changes look fine to me, but it should probably be documented in the changelog as a breaking change with 3.0.0.

[steveluscher]

The changes look fine to me, but it should probably be documented in the changelog as a breaking change with 3.0.0.

Wait, what on Earth? Why are these structs pub? They're literally only used in this file as a serialization schema, and immediately fed to serde_json. No Rust consumer ever sees them, which means it's not a breaking change; it's an addition to the JSON-RPC API, which is always safe.

Everything still typechecks when you remove pub from these structs. There's no evidence that anyone has used them on public GitHub.

[buffalojoec]

Wait, what on Earth? Why are these structs pub? They're literally only used in this file as a serialization schema, and immediately fed to serde_json. No Rust consumer ever sees them, which means it's not a breaking change; it's an addition to the JSON-RPC API, which is always safe.

Yeah they are all unfortunately leaked to the public API.

agave/rpc-client-api/src/lib.rs

Line 4 in 78d5954

pub mod custom_error;

Also, it changes the object structure in the data field of the JSON-RPC error, one of which goes from None (null) to Some.

https://www.jsonrpc.org/specification#error_object

Everything still typechecks when you remove pub from these structs. There's no evidence that anyone has used them on public GitHub.

Okay, but it's really no big deal to just add a changelog entry! We're on 3.0.0 right now.

[anza-team]

😱 New commits were pushed while the automerge label was present.

[steveluscher]

Except, of course, from the Rust perspective, I legit don't understand why adding a property to the error data is breaking, nor why going from no data to some data is breaking, but nevertheless I added a CHANGELOG entry that makes that claim.

[buffalojoec]

Except, of course, from the Rust perspective, I legit don't understand why adding a property to the error data is breaking, nor why going from no data to some data is breaking, but nevertheless I added a CHANGELOG entry that makes that claim.

Rust is strongly typed, so adding a new field to a struct breaks previous versions of that struct that did not have this field. If you have a struct, you might initialize it like this:

struct MyInfo {
  pub id: u64,
  pub count: u64,
}

let _ = MyInfo { id: 0, count: 0 };

Whenever there's suddenly a new field, you'd get a compiler error telling you that you're missing a field in the initialization. Furthermore, APIs like serde are hyper-sensitive to exact matches in parsing, so attempting to parse a stringified object with more keys than your struct could throw an error with out-of-the-box serde deserialization.

Similarly, Going from None to Some on an Option<T> where T is unchanged is not necessarily a breaking change from the Rust perspective, but it would change the JSON payload from a stateless null to a value, so anything that relies on ignoring a null/None may get tripped up. This one is less serious though, IMO.

[steveluscher]

I totally get all that about Rust. Very seriously though, I am confident that there are zero people constructing this struct in the wild, and that making it pub was just a mistake. The sole reason for the struct's existence is to serve as a schema for JSON serialization, and the only thing that it's ever used for is to serialize data to JSON internally. Never is an instance of that struct returned from any of the APIs, and in no place is that struct specified as an input.

The sole reason for the struct's existence is to serve as a schema for JSON serialization…

Like, literally, we could probably replace the whole thing with json!({ "slot": slot }), delete the struct, and le voilà.

[steveluscher]

Like, literally, we could probably replace the whole thing with json!({ "slot": slot }), delete the struct, and le voilà.

Actually, I feel strongly enough about not repeating the same mistake over again, that I pushed 0ab1060, even though it requires another review.

[buffalojoec]

I totally get all that about Rust. Very seriously though, I am confident that there are zero people constructing this struct in the wild, and that making it pub was just a mistake. The sole reason for the struct's existence is to serve as a schema for JSON serialization, and the only thing that it's ever used for is to serialize data to JSON internally. Never is an instance of that struct returned from any of the APIs, and in no place is that struct specified as an input.

The sole reason for the struct's existence is to serve as a schema for JSON serialization…

Like, literally, we could probably replace the whole thing with json!({ "slot": slot }), delete the struct, and le voilà.

I know, but it's mostly because of this:

agave/docs/src/backwards-compatibility.md

Line 60 in f915e9d

### RPC API

If you want, since it's a major release, you could see about just making the whole module private now.

[steveluscher]

If you want, since it's a major release, you could see about just making the whole module private now.

Oh no. What. Why?

agave/rpc-client/src/http_sender.rs

Lines 203 to 208 in f915e9d

	match serde_json::from_value::<custom_error::NodeUnhealthyErrorData>(json["error"]["data"].clone()) {
	Ok(custom_error::NodeUnhealthyErrorData {num_slots_behind}) => RpcResponseErrorData::NodeUnhealthy {num_slots_behind},
	Err(_err) => {
	RpcResponseErrorData::Empty
	}
	}

[steveluscher]

Alright, here's a compromise.

1. Made slot an Option in the existing type, and demonstrated through tests that this allows it to deserialize data from old RPCs
2. Didn't create a public struct for the new type, because why make the next person go through all of this for no reason.