Implement binary_search for collections::Vec

[KaiserKarel]

This ports the binary_search implementation from core. I've removed the use of get_unchecked as that is not present in LazyIndexMap. There's 2 tests added to verify that it works with a full vector and an empty vector.

related to #834

[KaiserKarel]

I think that the spellcheck failure is a false alarm, as the documentation is taken from core.

[Robbepop]

All in all LGTM so far. Will take another more detailed look into the actual implementation. We could use a bit more tests for this to cover some edge cases. Maybe pull some tests over from the core implementation?

[KaiserKarel]

All in all LGTM so far. Will take another more detailed look into the actual implementation. We could use a bit more tests for this to cover some edge cases. Maybe pull some tests over from the core implementation?

I originally was unable to find them, but now realize they resided in a different crate. I've added the core tests as well now, and removed my own tests.

[KaiserKarel]

Also @Robbepop, sinc binary_search and partition are quite closely related, should I add that one too? Or perhaps create another issue and another PR?

[Robbepop]

Also @Robbepop, sinc binary_search and partition are quite closely related, should I add that one too? Or perhaps create another issue and another PR?

Another API should be done in another PR. :)

[Robbepop]

Doc comments for the functions need to be adjusted to fit ink! instead of Rust standard library. It currently contains dead links and invalid docs or docs that just does not fit. Please rework docs in this regard and we are good to go. Also we might be in need of a sort function for storage::Vec in the near future. Although sorting efficiently will be a major endavour and might probably be a bad idea.

[KaiserKarel]

Doc comments for the functions need to be adjusted to fit ink! instead of Rust standard library. It currently contains dead links and invalid docs or docs that just does not fit. Please rework docs in this regard and we are good to go. Also we might be in need of a sort function for storage::Vec in the near future. Although sorting efficiently will be a major endavour and might probably be a bad idea.

For sorting it would probably be for efficient to read the storage vec into a normal vec, sort that, and write the result back? That should minimize the amount of gas fees I think.

Does a wishlist exist for additions to the ink stdlib?

[Robbepop]

For sorting it would probably be for efficient to read the storage vec into a normal vec, sort that, and write the result back? That should minimize the amount of gas fees I think.

Maybe yes, however due to caching in the LazyIndexMap layer this might not be needed. Benchmarks are required here instead of assumptions about performance.

[KaiserKarel]

Alright, so a PR involving adding sorting would require benchmarks of the two competing implementations?

[Robbepop]

LGTM
@cmichi please review/approve too.

We need to check the spellcheck CI so that our full pipeline is run on this PR before we merge.

Just add comparator to the spellcheck list for hunspell and the spellcheck CI should be fine with this PR.

[HCastano]

Some small nit picks, but after they're resolved we can merge

[KaiserKarel]

We should leave the first part of the comment ("it is ok to break...") here, it's just the crater part that's not relevant

Actually now that I think about it, wasn't binary_search implementation specific behaviour the reason that polkadot had an outage? Perhaps it is not okay for this test to break, as contracts may rely on the specific behaviour?

[HCastano]

Actually now that I think about it, wasn't binary_search implementation specific behaviour the reason that polkadot had an outage? Perhaps it is not okay for this test to break, as contracts may rely on the specific behaviour?

Umm kinda. The problem with Polkadot was that the Wasm runtime and native runtime had different sorting behaviours which caused a failure in consensus. You can read more about it here.

In our case once a contract is deployed that's the only instance of it. There is no need for a network of people with potentially different versions of the contract to come to consensus on the behaviour - they only need to run what's already on chain.

Whether or not we want to make any guarantees around sorting behaviour is an interesting topic. I'm not sure what the answer is here though. @cmichi @Robbepop what do you guys think?

[Robbepop]

Whether or not we want to make any guarantees around sorting behaviour is an interesting topic. I'm not sure what the answer is here though. @cmichi @Robbepop what do you guys think?

We should do the same as Rust and provide no sorting behavior since this could in theory severely reduce the efficiency of the algorithm. Furthermore, the API clearly says this and the problem for Polkadot existed because Polkadot did not obey the API calling guarantees and took something for granted that simply did not exist. So the problem was not caused by the Rust API or implementation but purely on the user side (Polkadot).

[Robbepop]

Please fix the doc examples to work with the respecitve ink_storage::Vec type so that we can merge this.