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

Modularize several data structures in core #4

Merged
merged 6 commits into from
Oct 23, 2016

Conversation

merope07
Copy link
Contributor

No description provided.

@merope07
Copy link
Contributor Author

This is a big PR. It contains no change in functionality, just moved around code, nor does it change any interfaces (except for changing their paths). Let me try to justify it a bit:

  • Right now the various core modules are coupled, but not in very strong or necessary ways. For example Block::verify uses internal members of TxProof, but it would probably be simpler for TxProof to expose an interface that can do what Block::verify needs. (Especially with an eye toward sinking signatures or Schnorr aggregation or whatever.)
  • Both Block and Transaction have functions called hash, while other data structures implement the Hashed trait, which is potentially confusing. It would be better that Hashed presented a more flexible API, in particular one that does not require allocations.
  • It's confusing to have ser and core::ser modules, I think.
  • In MerkleRow there was a call to the private function sha3 which is otherwise only used inside the default implementation of Hashed, exposing the internal hash function in one (and only one!) place it didn't quite belong.

These issues are things I identified while modularizing the code, and would have been hard to identify otherwise. Ultimately the way to modularize the code is up to the project lead, and is a bit of a style issue, but I think this scheme will lead to less technical debt. Note that Rust's only encapsulation/privacy boundary is the module, if things share a module then they can access each others' insides.

The most controversial thing I did was moving all of core/ser.rs elsewhere: the AsHashedBytes stuff to ser.rs and the structure serialization to the specific structures' modules. I think this is appropriate since the serialization will, in general, need access to private member variables.

Thoughts or comments (or rejection) would be welcome.

@ignopeverell
Copy link
Contributor

Thanks a lot for the PR, all very good cleanup. I had it in my list to go back to the core module and clean it up, so I'm really glad you tackled it!

I agree with most of your changes, separating out block.rs and transaction.rs particulary. Including the serialization code close to the implementation seems fair as well. I was also mulling around with the idea of a separate crypto module for all the Merkle/Hash/Commitment stuff.

My only change request is very minor: I'd prefer the MAX_IN_OUT_LEN constant in transaction.rs, I think it'd make more sense there. The ser.rs module is going to be reused for p2p message serialization and that constant makes little sense in there.

@merope07
Copy link
Contributor Author

Thanks :). Moved the constant.

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 this pull request may close these issues.

2 participants