feat: certs part 2 #14
Conversation
ruseinov
requested review from
lemmih and
LesnyRumcajs
and removed request for
a team
| @@ -141,9 +150,9 @@ mod tests { | |||
| let step = Phase::Commit; | |||
| let supplemental_data = SupplementalData { | |||
| commitments: keccak_hash::H256::zero(), | |||
| power_table: vec![], | |||
| power_table: Cid::default(), | |||
There was a problem hiding this comment.
Would a possible lack of power table be better represented by Option?
There was a problem hiding this comment.
It's not represented as an option in Go version (in this case a reference), so I don't think we should either. What they do is they check if the Cid is empty, just like us. I think it makes sense. Because an empty CID is not necessarily None.
There was a problem hiding this comment.
Default CID means that there's no power table, correct? The default CID still has a set multihash and version and, subjectively, checking if something is not of certain value to assert it's not there is bug-prone. If it's an Option then the developer knows that they need to check whether it exists and not deduce that they need to check against a particular hard-coded CID.
Not a terribly strong feeling, but I'd advise to reconsider the approach. It's similar to an Address - it can be defaulted to something, but what is the meaning of it? It's the reason that it's hidden in FVM under the testing feature. It's similar thing to a checksum of something - having a checksum of 00000 would mean (to me) that you actually ran the digest method and you somehow obtained all zeroes therefore you should quit your job and buy yourself an island from the billions you made on crypto. :)
There was a problem hiding this comment.
In our case we just use Cid::default() as we would any other Cid value - simply to fill the property with something. It's completely irrelevant for this test and reads better than a hardcoded string.
It isn't nullable in the reference code, so it should not be nullable here either.
There was a problem hiding this comment.
If we look through the codebase - it's also not being used for anything rather than comparison to another cid that's manually hashed, so there is no reason to change the approach.
scripts/linters/find_unused_deps.rb
Outdated
| # we are using a trick to enable a `test-utils` feature in the `f3-gpbft` which | ||
| # triggers a false positive. | ||
| crate == 'filecoin-f3-gpbft' |
There was a problem hiding this comment.
nit: why not crate == 'quickcheck' && crates.include?('quickcheck_macros') or crate == 'filecoin-f3-gpbft'?
There was a problem hiding this comment.
I think it's much more readable like it is right now. If we need to introduce more deps - perhaps we have to revisit the approach. Having a long chain of && and or isn't much better IMHO. No strong opinion.
There was a problem hiding this comment.
Might I suggest:
[
# `quickcheck` is required implicitly by `quickcheck_macros`
crate == 'quickcheck' && crates.include?('quickcheck_macros'),
# we are using a trick to enable a `test-utils` feature in the `f3-gpbft` which
# triggers a false positive.
crate == 'filecoin-f3-gpbft'
].any??
| @@ -37,7 +38,34 @@ impl Ord for PowerEntry { | |||
| } | |||
| } | |||
|
|
|||
| impl Serialize for PowerEntry { | |||
There was a problem hiding this comment.
Possible to use serde_tuple to avoid the manual implementations?
There was a problem hiding this comment.
There is a validation there as well to match the Go code. But we can at least derive deserialize.
certs/Cargo.toml
Outdated
| @@ -5,5 +5,10 @@ edition = "2021" | |||
|
|
|||
| [dependencies] | |||
| ahash = { workspace = true } | |||
| anyhow = { workspace = true } | |||
There was a problem hiding this comment.
This is a dev-dependency, I think.
gpbft/Cargo.toml
Outdated
| @@ -9,10 +9,20 @@ edition.workspace = true | |||
|
|
|||
| [dependencies] | |||
| ahash = { workspace = true } | |||
| anyhow = "1.0.86" | |||
| anyhow = { workspace = true } | |||
Co-authored-by: David Himmelstrup <lemmih@gmail.com>
Co-authored-by: David Himmelstrup <lemmih@gmail.com>
Closes ChainSafe/forest#4693
This is now only missing the verification bit as we don't have the crypto library yet. I think we can close this issue, I will introduce a new one for that piece.
UPD: ChainSafe/forest#4887