imp: Implement custom JSON and Borsh deserialization for ChainId
#1013
Conversation
seanchen1991
changed the title
ChainIdChainId
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1013 +/- ##
==========================================
+ Coverage 70.71% 70.90% +0.18%
==========================================
Files 178 178
Lines 17861 17995 +134
==========================================
+ Hits 12631 12759 +128
- Misses 5230 5236 +6 ☔ View full report in Codecov by Sentry. |
| let Ok((_, rn)) = parse_chain_id_string(&inner.id) else { | ||
| return Err(Error::new(ErrorKind::Other, "failed to parse chain ID")); | ||
| }; |
There was a problem hiding this comment.
Despite not being parsed, a chain id string is not necessarily invalid. The #[case(r#"{"id":"foo","revision_number":"0"}"#)] should be successfully deserialized.
Could you add tests for valid borsh desrialization cases as well?
There was a problem hiding this comment.
What about {"id":"foo-bar-42","revision_number":"0"}? The condition below accepts it but my intuition is that it should be rejected as well. IMO deserialised object should be the same as one constructed via ChainId::from_str(&deserialised.id).
There was a problem hiding this comment.
It's possible to have ChainIds that don't strictly follow the format, but are still considered valid. {"id":"foo-bar-42","revision_number":"0"} is an example of a ChainId that is valid but that doesn't strictly follow the format.
.changelog/unreleased/improvements/996-custom-chain-id-deserialization.md
Outdated
Show resolved
Hide resolved
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| const FIELDS: &[&str] = &["id", "revision_number"]; |
There was a problem hiding this comment.
Why not something like:
#[derive(serde::Deserialize)]
struct IdAndRevision {
id: String,
revision_number: u64,
}
let IdAndRevision { id, revision_number } =
IdAndRevision::deserialize(D)?;
if revision_number == parse_revision_number(chain_id.as_str())? {
Ok(Self { id, revision_number })
} else {
Err(...)
}And add fn parse_revision_number(id: &str) -> Result<u64, IdentifierError>; function
There was a problem hiding this comment.
It's not really clear to me which part of the serde::Deserialize impl you're thinking your suggested code should replace.
There was a problem hiding this comment.
What I’ve posted would be body of the deserialize method, i.e.:
#[cfg(feature = "serde")]
impl<'de> Deserialize<'de> for ChainId {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
D: Deserializer<'de>,
{
#[derive(Deserialize)]
struct IdAndRevision {
id: String,
revision_number: u64,
}
let IdAndRevision { id, revision_number } =
IdAndRevision::deserialize(deserializer)?;
if revision_number == 0 {
// If `revision_number` is zero we allow whatever valid identifier.
// In particular, `{id: "foo-bar-42", revision_number: 0}` is valid.
validate_chain_id(id.as_str())?;
} else if revision_number != parse_revision_number(id.as_str())? {
return Err(...);
}
Ok(Self { id, revision_number })
}
}There was a problem hiding this comment.
Still IdAndRevision::deserialize(deserializer)? is not working properly for all cases. Anyhow let’s leave further optimization for later PRs.
There was a problem hiding this comment.
What’s the issue? IdAndRevision with a derive should have the exact same deserialisation logic as ChainId.
| let Ok((_, rn)) = parse_chain_id_string(&inner.id) else { | ||
| return Err(Error::new(ErrorKind::Other, "failed to parse chain ID")); | ||
| }; |
There was a problem hiding this comment.
What about {"id":"foo-bar-42","revision_number":"0"}? The condition below accepts it but my intuition is that it should be rejected as well. IMO deserialised object should be the same as one constructed via ChainId::from_str(&deserialised.id).
…bc-rs into sean/verify-chain-id-de
There was a problem hiding this comment.
Thanks @seanchen1991 for sorting this out, and a nod to @mina86 for chiming in!
…1013) * Add ChainId json deserialize test * Add BorshDeserialization test * Add some debugging to ChainId deserialize impl * Change assertion to unwrap * First stab at custom Visitor and Deserialize impls * Implement custom Deserialize for ChainId * Remove unnecessary ChainId::from_str call * Add some additional assertions to test valid ChainIds * Stub out custom BorshDeserialize impl * Get custom borshDeserialize impl compiling * Add rstest test case testing invalid borsh deserialization * Add changelog entry * Cargo fmt * Incorporate some PR feedback * Remove expanded.rs file * Remove expanded.rs file * Clean up borshDeserialize impl * Add test the verifies valid borsh deserialization * Update BorshDeserialize test * test: add test_valid_borsh_ser_de_roundtrip * Clean up * nit * Move `use core::fmt` statement into serde::Deserialize impl * fix: core::fmt::Result --------- Co-authored-by: Farhad Shabani <[email protected]>
Closes: #996
Description
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.