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

fix(indexer): fixes inability to serialize Connection because of Arc. #1253

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

humb1t
Copy link
Contributor

@humb1t humb1t commented Jan 21, 2025

Description

Removes an Arc in Connection type.

Motivation and Context

With Arc type can't derive Serialize trait implementation.

How Has This Been Tested?

CI tests.

What process can a PR reviewer use to test or verify this change?

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

@sdbondi
Copy link
Member

sdbondi commented Jan 21, 2025

Thanks for the PR - Serialize is already implemented without compile errors. What specific problem existed that led to this change?

Not opposed to this change, but out of curiosity, I'd like to know a reason for it.

@humb1t
Copy link
Contributor Author

humb1t commented Jan 21, 2025

Strange @sdbondi - because I had this error recently:

error[E0277]: the trait bound `Arc<std::string::String>: Serialize` is not satisfied
    --> /Users//.cargo/git/checkouts/tari-dan-0943c0f9cd9028c7/996d28f/clients/tari_indexer_client/src/types.rs:440:10
     |
440  | #[derive(Serialize, Debug)]
     |          ^^^^^^^^^ the trait `Serialize` is not implemented for `Arc<std::string::String>`
...
460  |     pub user_agent: Option<Arc<String>>,
     |     ----------------------------------- required by a bound introduced by this call
     |
     = note: for local types consider adding `#[derive(serde::Serialize)]` to your `Arc<std::string::String>` type
     = note: for types from other crates check whether the crate offers a `serde` feature flag
     = help: the following other types implement trait `Serialize`:
               &'a T
               &'a mut T
               ()
               (T,)
               (T0, T1)
               (T0, T1, T2)
               (T0, T1, T2, T3)
               (T0, T1, T2, T3, T4)
             and 704 others
     = note: required for `std::option::Option<Arc<std::string::String>>` to implement `Serialize`
note: required by a bound in `types::_::_serde::ser::SerializeStruct::serialize_field`
    --> /Users//.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/serde-1.0.217/src/ser/mod.rs:1867:21
     |
1865 |     fn serialize_field<T>(&mut self, key: &'static str, value: &T) -> Result<(), Self::Error>
     |        --------------- required by a bound in this associated function
1866 |     where
1867 |         T: ?Sized + Serialize;
     |                     ^^^^^^^^^ required by this bound in `SerializeStruct::serialize_field`
     = note: this error originates in the derive macro `Serialize` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `tari_indexer_client` (lib) due to 1 previous error

Maybe it was the problem with misconfiguration of dependencies, or their caches.

@sdbondi
Copy link
Member

sdbondi commented Jan 21, 2025

Could be a serde version mismatch yeah, but happy to merge if this increases compatibility with more serde versions. Just need the clippies fixed

Copy link

Test Results (CI)

585 tests  ±0   585 ✅ ±0   1h 27m 13s ⏱️ + 8m 27s
 54 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 45f203d. ± Comparison against base commit 996d28f.

@sdbondi sdbondi changed the title fix(indexer): Fixes inability to serialize Connection because of Arc. fix(indexer): fixes inability to serialize Connection because of Arc. Jan 22, 2025
@sdbondi sdbondi merged commit f9804c0 into tari-project:development Jan 23, 2025
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants