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

Rust Bindings: Replacing slices of references to iterators of references for aggregation #198

Open
Coca162 opened this issue Dec 2, 2023 · 3 comments · May be fixed by #199
Open

Rust Bindings: Replacing slices of references to iterators of references for aggregation #198

Coca162 opened this issue Dec 2, 2023 · 3 comments · May be fixed by #199

Comments

@Coca162
Copy link

Coca162 commented Dec 2, 2023

Currently, functions like aggregate require a slice of references as input, however these functions do not necessarily require a slice and can be easily changed to accept any iterator instead. I'd be interested in making a PR for this where AggregatePublicKey and AggregateSignature's aggregation functions replace &[&[T] with impl IntoIterator<Item = &T>. This would require some users of the library to change code, but allows them to avoid allocating to a vector, providing a performance benefit and simplifying code.

@dot-asm
Copy link
Collaborator

dot-asm commented Dec 3, 2023

Formally speaking it's unreasonable to expect that users would have to modify their code unless we also increase the non-minor version. One can discuss that, but meanwhile why not start by suggesting an additional interface, say aggregate_iter? This would allow us to weigh pros and cons and maybe extend the approach...

@Coca162
Copy link
Author

Coca162 commented Dec 3, 2023

Having a aggregate_iter would also work, and it would allow for deprecation warnings to be put on current aggregate if it is decided to be removed in the future.

As for how users will have to modify their code if it's replaced, I think the only scenario will be one like this:

fn test() {
    let references_collection: [&usize; 4] = [&0, &0, &0, &0];

    // Works
    slice_of_ref(&references_collection);

    // Does not work, is a IntoIterator<Item = &&usize>
    into_iter_of_ref(&references_collection);
}

fn slice_of_ref(slice: &[&usize]) { }

fn into_iter_of_ref<'a>(slice: impl IntoIterator<Item = &'a usize>) { }

And can be fixed in either of these 2 ways:

// Consume collection, most likely not ideal
 into_iter_of_ref(references_collection);

// Copy to remove one reference
into_iter_of_ref(references_collection.iter().copied());

This could be possibly be remedied using some other generic trait, though I don't know of such way currently, and is most likely is not possible in current rust.

@Coca162
Copy link
Author

Coca162 commented Dec 3, 2023

unless we also increase the non-minor version

Since the crate is currently on major version 0 it is allowed to do breaking changes on minor versions:

Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.

https://semver.org/#spec-item-4

@Angel-Petrov Angel-Petrov linked a pull request Dec 7, 2023 that will close this issue
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 a pull request may close this issue.

2 participants