Skip to content

Conversation

UMR1352
Copy link
Contributor

@UMR1352 UMR1352 commented Aug 13, 2025

Description of change

This PR provides new APIs to query the DIDs controlled by a given address. These APIs also come in a streamed variant that returns a stream - i.e async iterator - instead of collecting the result into a list.

Add methods:

  • IdentityClientReadOnly::streamed_dids_controlled_by;
  • IdentityClientReadOnly::dids_controlled_by;
  • IdentityClient::streamed_controlled_dids;
  • IdentityClient::controlled_dids;

@UMR1352 UMR1352 requested a review from wulfraem August 13, 2025 14:40
@UMR1352 UMR1352 self-assigned this Aug 13, 2025
@UMR1352 UMR1352 added Enhancement New feature or improvement to an existing feature Wasm Related to Wasm bindings. Becomes part of the Wasm changelog Added A new feature that requires a minor release. Part of "Added" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. labels Aug 13, 2025
@UMR1352 UMR1352 marked this pull request as ready for review August 14, 2025 10:23
@eike-hass eike-hass moved this to Sprint Backlog in IOTA Trust Framework Developments Aug 15, 2025
@eike-hass eike-hass moved this from Sprint Backlog to In Review in IOTA Trust Framework Developments Aug 15, 2025
@UMR1352 UMR1352 changed the base branch from main to release/v1.7 August 18, 2025 11:47
@UMR1352 UMR1352 force-pushed the feat/controlled-dids branch from d09777f to 0262250 Compare September 1, 2025 09:10
@UMR1352 UMR1352 changed the base branch from release/v1.7 to main September 1, 2025 09:13
@UMR1352 UMR1352 changed the base branch from main to release/v1.7 September 1, 2025 09:14
/// # Errors
/// This method might return a [QueryControlledDidsError] when the underlying RPC call fails.
/// [QueryControlledDidsError]'s source can be downcasted to [SDK's Error](iota_interaction::error::Error)
/// in order to check whether calling this method again might return a successfull result.
Copy link
Contributor

Choose a reason for hiding this comment

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

^^

Suggested change
/// in order to check whether calling this method again might return a successfull result.
/// in order to check whether calling this method again might return a successful result.

/// # Ok(())
/// # }
/// ```
pub async fn dids_controlled_by(&self, address: IotaAddress) -> Result<Vec<IotaDID>, QueryControlledDidsError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, kind of mixed feelings about having both functions, tbh. So some more or less random thoughts about it... :D

Also, as they all kind of belong together, I put them in the same comment, but let's assign them numbers, to make things easier to address in a conversation. :D

  1. Do we need both functions, or could we drop the non-streamed one? We could also add a comment for how to read all entries at once to the streamed version to give people a hint on how to do this, if they want. Maybe we need it to keep the behaviors consistent between the products and/or Rust/WASM, but looking only at identity, I don't see too much of a benefit for keeping both.

  2. If we want to keep both, would it make sense to add the streamed part as a suffix, e.g. dids_controlled_by_streamed, which might make the first part of the function more findable when working with the API for the first time. This would also match the pattern in the full client (or we'll adjust the functions there, to make them match), current functions:

IdentityClientReadOnly dids_controlled_by
IdentityClientReadOnly streamed_dids_controlled_by
IdentityClient controlled_dids
IdentityClient controlled_dids_streamed
  1. Maybe personal assumption, but I usually expect prefix-less getters (e.g. dids_controlled_by) to be pretty simple functions with a small footprint, which dids_controlled_by might not always be, therefore would have expected the two functions to have a get_ prefix (e.g. get_dids_controlled_by), but this may also be just an assumption on my side. ^^;;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm down to only provide a streamed version of the API, though it's not very ergonomic to use as users have to first pin the stream before they can consume it. Besides, I hate we have to rely on an external library for this, but I don't see any other way, since std::async_iter seems to be far from stabilization.

@wulfraem what do you think? The pinning drawback isn't insurmountable, users will have to either pin! or Box::pin their stream, if you think that's acceptable I'll proceed with removing the non_streaming version of the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, how would using the streaming version look like on the consuming side, like in the example, where I overlooked the pin (thank you for providing that, just missed the pion >.<)?

My initial naive assumption was, that the following two would be exchangeable:

let dids_from_non_streamed = ro_client
  .dids_controlled_by(address)
  .await?;

let dids_collected_from_stream = ro_client
  .streamed_dids_controlled_by(address)
  .try_collect() // `.next()`, etc.
  .await?;

But yeah, that pinning feels a bit disjunct and out of the wild, but it's still possible to use it for custom implementations. It just feels like people relying too heavy on the non-streamed version might be in for an unpleasant surprise, if they have too many DIDs on the same account address, but one could also run into the same behavior when using the streamed version without considering that. >.<

So, let's keep both, and ignore point 1. What about point 2 and 3?

Imo for 2 it would be nice to have the 2 streamed function names aligned.

And for 3, as mentioned, my assumption would be that a prefix-less getter would not try to fetch an unpredictable amount of result pages, but would also be fine with it, if we go for prefix-less for the sake of consistentcy. ^^

Copy link
Contributor Author

@UMR1352 UMR1352 Sep 3, 2025

Choose a reason for hiding this comment

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

My initial naive assumption was, that the following two would be exchangeable.

That would work but only because try_collect pins the stream itself. If a user wants to instead consume the stream item by item (by calling next) they'd have to pin it themselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. Yep, then the manual pinning makes it a bit more clunky. >.<

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added A new feature that requires a minor release. Part of "Added" section in changelog Enhancement New feature or improvement to an existing feature Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants