Fixed seed conditions and added tests for applying transactions. #126
Conversation
…ir ids in the seed source.
…tion body; added tests for applying transactions.
tx.go
Outdated
| return &tx | ||
| } | ||
|
|
||
| func AttachSenderToTransactionInPlace(sender *skademlia.Keypair, tx *Transaction, parents ...*Transaction) { |
There was a problem hiding this comment.
Rename to AppendSenderToTransaction to imply that it in-place modifies the transaction provided. Also please provide documentation on these functions.
There was a problem hiding this comment.
I don't think Append and Attach really imply this difference.
There was a problem hiding this comment.
The Append prefix pattern, I am taking it as a style guideline from the fasthttp, fastjson etc libraries that provide functions which modify some pointer in-place.
tx.go
Outdated
| t.SeedLen = byte(prefixLen(t.Seed[:])) | ||
| _, err = hasher.Write(t.Sender[:]) | ||
| if err != nil { | ||
| panic(err) // should never happen |
There was a problem hiding this comment.
If it should never happen, then write instead _, _ = hasher.Write(t.Sender[:]).
There was a problem hiding this comment.
Same goes for other error-check statements within rehash().
There was a problem hiding this comment.
i would put log even in case it will "never" happen.
There was a problem hiding this comment.
If something that should never happen does happen, it should be a fatal error. Panic in this case.
There was a problem hiding this comment.
I'd argue just explicitly ignoring via _, _ = hasher.Write(t.Sender[:]). The documentation states that an error will never be thrown. A panic nor an error in this case should never have to occur. If needed, a comment can be appended to emphasize this.
…ssed comments on PR and added more tests for specific transaction types.
…ions and fixed contract execution in Transfer transactions.
…ction application.
|
FIXME(@iwasaki-kenta): Whitepaper must be updated with respect to the new seed conditions. |
|
One more thing on topic about pointers vs values. It would've been good to have benchmark if we care about speed. Because every struct value which is not passed by pointer is allocated on the stack which is much faster then the heap. So maybe its actually cheaper to allocate new struct for each function call on the stack then to allocate it on the heap and then resolve pointer (which is also not for free). |
I completely agree as well. Originally transactions were only heap-allocated should they be added to the graph of a node. We should setup performance regression tests in the near future. I will also adjust some styling in this PR after merging it in to master given that all tests pass and nothing breaks. |
No description provided.