Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add serialization traits to Status #499

Merged
merged 15 commits into from
Feb 13, 2024
Merged

Conversation

bitdivine
Copy link
Member

@bitdivine bitdivine commented Dec 1, 2023

Description

We would like to express the Status response printed by dfx as IDL and/or JSON. This requires corresponding serialization traits to be implemented for Status.

Contributes towards fixing: https://dfinity.atlassian.net/browse/SDK-1319

How Has This Been Tested?

Unit tests are included for serialization to JSON and IDL.

Checklist:

  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.
    • Not sure what is needed here, if anything. The traits should show up in the generated rustdoc and that is the documentation I look at most frequently when I wish to know how to serialize a type. On that basis I believe this is done.

@bitdivine bitdivine changed the title Add serialization traits to Status feat: Add serialization traits to Status Dec 6, 2023
@bitdivine bitdivine marked this pull request as ready for review December 6, 2023 13:14
@bitdivine bitdivine requested a review from a team as a code owner December 6, 2023 13:14
@bitdivine
Copy link
Member Author

Note: The cla test is hanging. Is this a known issue?

@@ -17,7 +17,7 @@ include = ["src", "Cargo.toml", "../LICENSE", "README.md"]
[dependencies]
backoff = "0.4.0"
cached = { version = "0.46", features = ["ahash"], default-features = false }
candid = { workspace = true }
candid = { workspace = true, features = ["value"] }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I am not super happy about enabling a feature in code that may be used as a library. IDLValue is needed for tests only, but AFAIK there is no way of enabling a feature for tests only. Maybe the test is overkill. What do you think?

Copy link

@smallstepman smallstepman Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK a different version of a crate can be added to dev-dependencies under an alias, but not the same version of the same crate with a different feature set. But I can try it just to be sure.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Let me know, I'm curious

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes: Experiment:

  • I dropped the "values" feature from the candid dependency.
  • I added a the candid crate under an alias in the dev dependencies.
  • cargo check complained about the duplicate:
max@sinkpad:~/dfn/agent-rs (14:27)$ git diff
diff --git a/ic-agent/Cargo.toml b/ic-agent/Cargo.toml
index 8308d2b..5b41a89 100644
--- a/ic-agent/Cargo.toml
+++ b/ic-agent/Cargo.toml
@@ -17,7 +17,7 @@ include = ["src", "Cargo.toml", "../LICENSE", "README.md"]
 [dependencies]
 backoff = "0.4.0"
 cached = { version = "0.46", features = ["ahash"], default-features = false }
-candid = { workspace = true, features = ["value"] }
+candid = { workspace = true }
 ed25519-consensus = { version = "2" }
 futures-util = "0.3.21"
 hex = { workspace = true }
@@ -75,6 +75,7 @@ web-sys = { version = "0.3", features = ["Window"], optional = true }
 
 [dev-dependencies]
 serde_json = "1.0.79"
+dev_candid = { package = "candid", version = "0.10.0", features = ["value"]}
 
 [target.'cfg(not(target_family = "wasm"))'.dev-dependencies]
 tokio = { version = "1.24.2", features = ["full"] }
diff --git a/ic-agent/src/agent/status.rs b/ic-agent/src/agent/status.rs
index e54ea25..a53d2f3 100644
--- a/ic-agent/src/agent/status.rs
+++ b/ic-agent/src/agent/status.rs
@@ -74,8 +74,8 @@ fn can_serilaize_status_as_json() {
 }
 #[test]
 fn can_serialize_status_as_idl() {
-    use candid::types::value::IDLValue;
-    use candid::{Encode, IDLArgs, Result as CandidResult, TypeEnv};
+    use dev_candid::types::value::IDLValue;
+    use dev_candid::{Encode, IDLArgs, Result as CandidResult, TypeEnv};
     let status = Status {
         impl_version: Some("Foo".to_string()),
         replica_health_status: None,
max@sinkpad:~/dfn/agent-rs (14:29)$ 
max@sinkpad:~/dfn/agent-rs (14:32)$ 
max@sinkpad:~/dfn/agent-rs (14:32)$ cargo check
error: the crate `ic-agent v0.31.0 (/home/max/dfn/agent-rs/ic-agent)` depends on crate `candid v0.10.0` multiple times with different names
max@sinkpad:~/dfn/agent-rs (14:38)$ 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this worked for me
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have looked at the "value" feature and don't really understand the repercussions of adding the feature. It changes some low level behaviour in corner cases. I think that the right answer in this context is to drop the test and not change the feature set. The CandidType and Deserialize traits are pretty well tested elsewhere so it's not likely that they will break here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I don't have strong opinion about adding value feature despite not being used in production code... maybe its ok and not worth the hassle to play the cargo here. Was only trying to be helpful ;)

Copy link
Member Author

@bitdivine bitdivine Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe silly question, but is it necessary to use the alias?

OOh, cool! I didn't know that was possible. Thanks! Yes, I think that is the way to go.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. CI got stuck. All green now apart from cla, which is hanging. Do you know how to un-stick that?

@ericswanson-dfinity ericswanson-dfinity enabled auto-merge (squash) February 13, 2024 23:21
@ericswanson-dfinity ericswanson-dfinity merged commit f216c74 into main Feb 13, 2024
24 checks passed
@ericswanson-dfinity ericswanson-dfinity deleted the serialize-status branch February 13, 2024 23:28
ericswanson-dfinity added a commit that referenced this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants