feat(dtos): add Participants JSON serialization types to contract-interface#1990
feat(dtos): add Participants JSON serialization types to contract-interface#1990
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds JSON serialization helper types to the contract-interface crate to support the upcoming changes in PR #1861, which will convert the Participants struct from a Vec-based to a BTreeMap-based structure for improved performance.
Changes:
- Adds a new
participants.rsmodule with DTO types for JSON serialization/deserialization - Adds
near-account-iddependency to contract-interface with serde feature - Exports the new participant types from the public API
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/contract-interface/src/types/participants.rs | New module defining DTO types (ParticipantId, ParticipantInfo, ParticipantData, ParticipantsJson, ParticipantsField, ParticipantsJsonDeserialize) for JSON wire format with backward compatibility |
| crates/contract-interface/src/lib.rs | Exports the new participant types in the public API |
| crates/contract-interface/Cargo.toml | Adds near-account-id dependency with serde feature |
| Cargo.lock | Updates dependency lockfile to include near-account-id for contract-interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This prepares for PR #1861 which changes the internal `Participants` storage from `Vec` to `BTreeMap`.
3e78459 to
58eeace
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
What I really like about this PR is that we have a unit test for backwards compatibility!
Blockers as of now:
- we should be using the type defined in
contract_interfacein the interface (this is the cumbersome part) - we should not be introducing anything more than what is strictly needed right now.
Some context on the contract_interface crate:
The purpose of that crate is to ensure backwards-compatibility (or, at least, make it much easier to spot backwards-breaking changes).
The goal is to have all types that occur as input or output of contract methods defined in that crate.
This allows us to modify node- or contract-internal types, without having to worry if it will break the interface.
In this PR, we want to:
- define a struct in the
contract_interfacecrate that will serialize to and deserialize from the same json format as the existingParticipantsstruct. This task is mostly achieved, as evidenced by the unit tests. But we should not introduce new types in this PR (one of the blockers). - ensure that we no longer serialize the
Participantsstruct in the contract. Instead, we need an.into_dto_type()method for the type that is used internally in the contract. It is that method, that will then later be modified in #1861.
The second task is a bit cumbersome in this case, because the Participants struct is used in ThresholdParameters, which occurs in our protocol contract states, which are returned in the state function (which must remain backwards compatible) - IIRC, none of these are yet in the contract_interface crate and might also need to be added.
As mentioned in the other PR, this is significant work, but we were bound to run into this issue sooner or later, so it's definitely worth doing!
Note: I do suspect this is a good task for an LLM. It's mostly boilerplate and the underlying patterns are simple.
gilcu3
left a comment
There was a problem hiding this comment.
Thank you! That's a lot of code 📈
I have mainly one blocker, but many questions and suggestions:
- The DomainId type was already defined in the dto crate, so no need to use the one from the contract there
Important points:
- There is an infallible conversion that was fallible before, please check if that's the case and keep the fallible one
- Clarify the purpose of the
ParticipantsJsonstruct
…d `Threshold` structs
… `&impl Serialize`
…` and `ThresholdParametersVotes` structs
gilcu3
left a comment
There was a problem hiding this comment.
Thank you for all the changes. I think we have mainly two remainings tasks that should probably be its own separate follow-ups.
- I am not sure we are testing if the current contract structs and the new dtos structs use the same json serialization. Those tests would need to be included in the contract afaik
- From #1990 (comment), I think we should eventually remove the PublicKeyExtended type from the dtos, as we already have PublicKey there for that purpose
Summary
Fixes #2001.
This PR addresses the code review feedback on #1861 (comment) by moving the JSON serialization helpers from the primitives module to the
contract-interface(DTOs) crate.Context
PR #1861 changes
ParticipantsfromVec<(AccountId, ParticipantId, ParticipantInfo)>toBTreeMap<AccountId, ParticipantData>for O(log n) lookups. To maintain JSON backward compatibility, it added serialization helpers directly in the primitives module. We’re now moving them to the DTOs crate.Next Steps
After this PR merges:
Participantsfrom aVec-based structure to aBTreeMapfor improved gas usage #1861 onmainparticipants.rsto import these types fromcontract_interface::types