Repatriate `arbitrary` implementations

ChainSafe / forest

🌲 Rust Filecoin Node Implementation

https://forest.chainsafe.io

Apache License 2.0

620 stars 146 forks source link

Repatriate `arbitrary` implementations #2130

Closed lemmih closed 1 year ago

lemmih commented 1 year ago

Issue summary

We wrap several types in order to implement the Arbitrary trait for them. Ideally we want the upstream libraries to natively support quickcheck. We should submit PRs that add quickcheck support under an optional feature.

[x] Cid: https://github.com/multiformats/rust-cid/pull/125
[x] Ipld: https://crates.io/crates/libipld-core
[x] ActorState: https://crates.io/crates/fvm
[x] Address, PoStProof, Message, Signature, SignatureType: https://crates.io/crates/fvm_shared

Other information and links

Ipld PR https://github.com/ipld/libipld/pull/167
fvm_shared PR https://github.com/filecoin-project/ref-fvm/pull/1037
fvm PR https://github.com/filecoin-project/ref-fvm/pull/1084 The fvm_shared Pr needs to be merged first before ActorState is implemented. The PR is ready to be opened in the meantime

lemmih commented 1 year ago

@connormullett Remember to add links to the PRs.

lemmih commented 1 year ago

@tyshko5 You want this task?

tyshko5 commented 1 year ago

@tyshko5 You want this task?

Sure. Only ActorState left, right?

lemmih commented 1 year ago

We also need to follow-up on the other PRs and make sure they are merged.

lemmih commented 1 year ago

@tyshko5 The ActorState implementations needs to be updated once the Address code has been merged.

lemmih commented 1 year ago

@tyshko5 Could you push this PR to completion: https://github.com/ipld/libipld/pull/157 ? If you're unsure of what to do, ask in our slack channel.

tyshko5 commented 1 year ago

@tyshko5 Could you push this PR to completion: ipld/libipld#157 ? If you're unsure of what to do, ask in our slack channel.

lemmih commented 1 year ago

Status:

IPLD: PR is done and will be merged soon: https://github.com/ipld/libipld/pull/167
fvm_shared: PR is done but requires buy-in from owners: https://github.com/filecoin-project/ref-fvm/pull/1037
fvm: A new PR needs to be created after fvm_shared is updated. @tyshko5 will create the new PR.

lemmih commented 1 year ago

The fvm_shared PR got merged. Please update the arbitrary implementation for ActorState, @tyshko5. Reply to my message with an emoji once you've seen this, know what you need to do, and have no blockers.

tyshko5 commented 1 year ago

https://github.com/filecoin-project/ref-fvm/pull/1162

lemmih commented 1 year ago

Blocked. More details: https://github.com/filecoin-project/ref-fvm/pull/1211

lemmih commented 1 year ago

This is no longer blocked with the release of FVM3.

@tyshko5 @hanabi1224 @creativcoder @sudo-shashank, feel free to take this if you're running low on tasks.

lemmih commented 1 year ago

(Partially unblocked - the cid crate still cannot be upgraded)

jdjaustin commented 1 year ago

It appears PoStProof was not included in the fvm_shared PR: https://github.com/filecoin-project/ref-fvm/pull/1037.

tyshko5 commented 1 year ago

(Partially unblocked - the cid crate still cannot be upgraded)

If cid still cannot be updated, then we still cannot repatriate ActorState

tyshko5 commented 1 year ago

filecoin-project/ref-fvm#1037

https://github.com/filecoin-project/ref-fvm/pull/1684

jdjaustin commented 1 year ago

mod arb is private. I believe it needs to be public, unless there is a workaround or I'm not using it correctly. https://github.com/ipld/libipld/blob/d7ac11d8433d3bf54e626382a2091ac63fccd577/core/src/lib.rs#L19

lemmih commented 1 year ago

mod arb is private. I believe it needs to be public, unless there is a workaround or I'm not using it correctly. https://github.com/ipld/libipld/blob/d7ac11d8433d3bf54e626382a2091ac63fccd577/core/src/lib.rs#L19

You should be able to access the implementations just fine.

lemmih commented 1 year ago

Done.