73 inline entry type

[thedavidmeister]

#73

this diverges from the legacy golang code which has fn signatures looking like this:

func (c *Chain) AddEntry(now time.Time, entryType string, e Entry, privKey ic.PrivKey) (hash Hash, err error) {

which is analogous to our chain.push() trait method in rust.

the limitations of the go version, that i see:

• fn signature is complex enough to be a code smell, prepareHeader is even worse
• managing the time and entry type separate to the Entry undermines the benefits of Entry being immutable
• the private key could come from an agent referenced by the chain itself, i think? i'm not 100% sure if it makes sense to have multiple private keys for a single chain that we select at push() time? @zippy can probably provide more context here.

changes:

• entry type is set on the entry as part of Entry::new()
  • old: Entry::new("content") new: Entry::new("type", "content")
• type is no longer provided to Header/Pair/Chain
  • old: chain.push("type", Entry::new("content")) new: chain.push(Entry::new("type", "content"))

pros:

• no longer possible to make a mistake by providing the wrong type for an entry at push time
  • avoids "hidden mutability" of Entry where the type is only provided after new() call
• can get the type from the entry
• much simpler method signatures across Header/Pair/Chain, notably chain.push()
  • can give similar treatment to time as well, locking down time at the actual Entry creation time

cons:

• doesn't line up with serialized representation (should be able to be handled by serde don't serialize entry_type into Entry (it should only be in Header) #80)
• small amount of duplicate type data in Pair across Entry/Header

[codecov-io]

Codecov Report

Merging #79 into develop will increase coverage by 0.96%.
The diff coverage is 94.28%.

@@             Coverage Diff             @@
##           develop      #79      +/-   ##
===========================================
+ Coverage    79.35%   80.32%   +0.96%     
===========================================
  Files           13       13              
  Lines          499      549      +50     
===========================================
+ Hits           396      441      +45     
- Misses         103      108       +5

Impacted Files	Coverage Δ
core/src/nucleus/ribosome.rs	67.18% <ø> (-2.17%)	⬇️
core/src/persister.rs	95.65% <100%> (+0.19%)	⬆️
core/src/chain/pair.rs	97.82% <87.5%> (+0.26%)	⬆️
core/src/chain/header.rs	97.45% <95%> (-1.68%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d69e81f...1aec0ef. Read the comment docs.

[sphinxc0re]

Since the standard Hash trait doesn't clash with anything in this context, could you remove the alias for now?

[thedavidmeister]

it clashes with Entry::hash() doesn't it?

[sphinxc0re]

It shouldn't. Not in this particular context.

[thedavidmeister]

@sphinxc0re it causes recursion issues if i try self.hash() but i can drop the alias, check out the latest commit

[sphinxc0re]

You could just #[derive(Hash)] this.

[thedavidmeister]

let's continue this in #20

[zippy]

Nice work. You inspire me in the thoroughness of your tests.

[zippy]

This deserves a little bit more in the comment to explain why, because at first glance you might think not. The reason is that what's hashed is JUST the content. i.e. the type is just a convenience value for the implementation, not part of what's getting hashed.

[thedavidmeister]

heh, i'm not even sure if this is desirable behaviour!

part of the reason for writing this test was to highlight something unintuitive about the code :/

i opened up #85 because intuitively the hash of an entry, and also its concept of equality, would contain its type

[thedavidmeister]

@zippy i added an @see to #85 to provide a pointer to the reasoning