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.

Sign up for GitHub

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

Jump to bottom

simple_hash_from_byte_slices should take a Vec instead of slice #57

Closed
ebuchman opened this issue Nov 6, 2019 · 3 comments · Fixed by #59
Closed

simple_hash_from_byte_slices should take a Vec instead of slice #57

ebuchman opened this issue Nov 6, 2019 · 3 comments · Fixed by #59

Comments

@ebuchman
Copy link
Member

ebuchman commented Nov 6, 2019 •
edited
Loading

Currently we have fn simple_hash_from_byte_slices(byte_slices: &[&[u8]]) -> Hash

But this leads to some complexities in the few places it's so far used:
@liamsi liamsi mentioned this issue Nov 8, 2019
Merged
@liamsi
Copy link
Member

liamsi commented Nov 8, 2019 •
edited
Loading

Although it looks like a good idea, we should also consider that in Rust it is "more common to pass slices as arguments rather than vectors when you just want to provide a read access"
(ref: https://doc.rust-lang.org/std/vec/struct.Vec.html#slicing)

There is even a clippy warning about fn(&Vec) which we'd need to disable then: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_arg

see for example #59 (clippy fails); passing in Vec<Vec<u8>> instead works though.

@ebuchman
Copy link
Member Author

ebuchman commented Nov 8, 2019

Oh hmm. I was thinking we'd want Vec<Vec<u8>>. It seems justified since if I'm not mistaken these arguments are not used again and so can be moved and consumed by this function.

That said for the recursion we may want an inner function that deals in slices so we don't need to_vec everywhere!

@tarcieri
Copy link
Contributor

tarcieri commented Nov 8, 2019

@ebuchman you can always add a From impl that consumes (or creates) a Vec

liamsi added a commit that referenced this issue Nov 9, 2019
@liamsi
WIP: pass in Vec<Vec<u8> instead of &Vec<Vec<u8>
e34f1b6 
 - work in progress as we might want to let an inner function for the
   recursion
 - also we might simply want to use From to consume (or create a Vec)
   instead of changing the method signature (as suggested here:
   #57 (comment))
tomtau pushed a commit to crypto-com/tendermint-rs that referenced this issue Nov 19, 2019
@yihuang @tomtau
Update crypto-com-chain branch for the new PR request (#3)
fb56737 
* build(deps): update tai64 requirement from 2 to 3 (informalsystems#22)

Updates the requirements on [tai64](https://github.com/iqlusioninc/crates) to permit the latest version.
- [Release notes](https://github.com/iqlusioninc/crates/releases)
- [Commits](iqlusioninc/crates@canonical-path/v2.0.0...tai64/3.0.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

* simple_hash_from_byte_slices takes a Vec instead of slice

* cargo fmt

* WIP: pass in Vec<Vec<u8> instead of &Vec<Vec<u8>

 - work in progress as we might want to let an inner function for the
   recursion
 - also we might simply want to use From to consume (or create a Vec)
   instead of changing the method signature (as suggested here:
   informalsystems#57 (comment))

* use Vec<Vec<u8>> with inner recursion method

* merge in changes of informalsystems#59 and simplify Header::hash

* simplify tests:
 direct vector initialization instead of going through &[&[u8]]

* remove obsolete comment

* Further reduce boilerplate code by leveraging generics

* max_gas should be i64, can be -1

* remove unneed clones

* remove secret_connection

* Create a trait for with a blanket impl instead of new type

Thanks for the suggestion @tarcieri :-)

* impl lite::Commit for commit::SignedHeader

* Parse validators field in genesis api

* Modify lite::Commit impl for SignedHeader to not include amino dep:

 - SignedVote can now be constructed without going through CanoncialVote
   (which is an amino-type)
 - modify impl to match new constructor and remove amino dep
 - make explicit where which types are amino_types (in vote.rs)

* impl lite::ValidatorSetLookup

* Add a few methods to tests and deduplicate code for fn hash of
lite::ValidatorSet:: & Set

- tests for  lite::ValidatorSetLookup lite::ValidatorSet impl
- delete `fn hash(self) -> merkle::Hash` from Set and only keep the impl
  of lite::ValidatorSet

* implement utility traits for tendermint data types

* Signing bytes need to encoded with encode_length_delimited

* utilities
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

simple_hash_from_byte_slices takes a Vec instead of slice informalsystems/tendermint-rs
3 participants
@tarcieri @ebuchman @liamsi