-
Notifications
You must be signed in to change notification settings - Fork 153
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
New Tipset w/ unit tests #56
Conversation
* Added multihash package to retrieve blake2b hash * Added sort_key method for Ticket struct * Added equals method for TipSetKeys struct type * WIP for Tipset new fn * WIP cid method for block type which would return CID of block
* Added new conditional checks as per spec update * WIP sorted and compare logic for ticket size * Fix cid method for blockHeader * WIP new fn tests
* Added unit tests for tipset methods
* Added unit tests for tipset methods
* Added new tipset logic, includes conditional checks and sorting based on ticket size * Added unit tests for tipset methods * Added cid fn although it is incomplete as we need cbor encoding
* Fixed equals fn check * Linted
* Updated multihash to specific version * Removed extern crate statement * Improved commenting * Updated sort statement * Improved unit test setup * Updated error messages for tipset condition checks
// push headers into vec for sorting | ||
sorted_headers.push(headers[i].clone()); | ||
// push header cid into vec for unique check | ||
cids.push(headers[i].clone().cid()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the cids don't have to be in sorted order? What are they used for if they can just be pulled from the headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't have to be sorted that was my mistake, but they need to be pushed into a vector to be used for an additional condition check to ensure each CID is unique and not duplicated. Currently, this is outlined as a TODO as we require fn cid
in block.rs
to be completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of a vector of the cids could just be a hash set? I'm okay with this though since efficiency isn't a priority
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it could be but I would suggest waiting to make that change until the other pieces required are completed.
* Updated cid reference * Removed pub from mod.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some cosmetics. Shouldn't need a method in Ticket to return clone of vrfproof. Moreover, even if you did, better naming than sort_key.
* Updated error to handle String * Removed unnecessary import * Removed sort_key method for Ticket
Requested changes made @ec2 @GregTheGreek |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Good stuff bro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
* Removed dead code and unused variable tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits and clarifications
Ignore the test case comments if you guys don't care, this setup just doesn't seem the best for indicating this does what it should and that if things change with the setup that the tests will still work
let data0: Vec<u8> = vec![1, 4, 3, 6, 7, 1, 2]; | ||
let data1: Vec<u8> = vec![1, 4, 3, 6, 1, 1, 2, 2, 4, 5, 3, 12, 2, 1]; | ||
let data2: Vec<u8> = vec![1, 4, 3, 6, 1, 1, 2, 2, 4, 5, 3, 12, 2]; | ||
let cids = key_setup(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this even used here when individual cids are passed in individually only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need unique cid for cached_cid
(individual cid
passed in), and the same cid between each header for state_root
, message_receipts
, and parent_cids
which is derived from another call to key_setup
const HEIGHT: u64 = 1; | ||
const CACHED_BYTES: u8 = 0; | ||
|
||
fn template_key(data: &[u8]) -> Cid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the point of this is to not rely on indexing the key_setup() so hard to follow what is being tested
@austinabell In an effort to try to explain the tests via your comments I have concluded that they're difficult to follow...although I believe they test correctly I am going to restructure and make them far more clear. |
Changes introduced in this pr:
new
which includes updated spec conditional checks and sorting based on ticket sizeequals
tipset_key
methodcid
for generating CID for headersTodo:
cid
fn and then will need to implement a cid check within thenew
fnCloses #8