Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add fn new_dag_cbor_default to Cid #2909

Merged
merged 10 commits into from
Jun 19, 2023
Merged

feat: add fn new_dag_cbor_default to Cid #2909

merged 10 commits into from
Jun 19, 2023

Conversation

hanabi1224
Copy link
Contributor

Summary of changes

As part of #2801

Changes introduced in this pull request:

  • Add extension method fn new_dag_cbor_default to Cid to match the default filecoin CID builder in go. See code

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@hanabi1224 hanabi1224 marked this pull request as ready for review May 24, 2023 16:36
lemmih
lemmih requested changes May 25, 2023
View reviewed changes
Copy link
Contributor

@lemmih lemmih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain a little more about why we want this?

@hanabi1224
Copy link
Contributor Author

Can you explain a little more about why we want this?

@lemmih In state migration code, sometimes it needs to calculate CID for a cor-serializable object, without calling CborStore.put_cbor_default API. One example is calculating CID for DealProposal (see ChainSafe/fil-actor-states#120), I feel it unsustainable to add a method to each struct, it would be much easier to do it with Cid::new_dag_cbor_default(deal_proposal)

LesnyRumcajs
LesnyRumcajs reviewed May 29, 2023
View reviewed changes
utils/forest_utils/src/cid/mod.rs Outdated

/// Extension methods for constructing `dag-cbor` [Cid]
pub trait CidCborExt {
/// Default CID builder for Filecoin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a lovely new trait! It would be a shame not to have 100% coverage on it! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test added

@LesnyRumcajs
Copy link
Member

@hanabi1224, what do we do with this PR?

lemmih
lemmih requested changes Jun 6, 2023
View reviewed changes
utils/forest_utils/src/cid/mod.rs Outdated Show resolved Hide resolved
utils/forest_utils/src/cid/mod.rs Outdated
/// - The default hash function is 256 bit BLAKE2b
///
/// This matches [`abi.CidBuilder`](https://github.com/filecoin-project/go-state-types/blob/master/abi/cid.go#L49) in go
fn new_dag_cbor_default<S: serde::ser::Serialize>(obj: &S) -> anyhow::Result<Cid> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming feels non-intuitive to me. I can see that the default part refers to blake256, but I don't see any non-default counterpart. How about naming this function from_cbor or from_cbor_blake256?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@LesnyRumcajs
Copy link
Member

@lemmih @hanabi1224, what do we do with this PR?

@lemmih
Copy link
Contributor

lemmih commented Jun 19, 2023

@lemmih @hanabi1224, what do we do with this PR?

I'm okay with merging it.

@hanabi1224 hanabi1224 requested a review from ruseinov as a code owner June 19, 2023 07:01
@hanabi1224 hanabi1224 enabled auto-merge (squash) June 19, 2023 07:03
@hanabi1224 hanabi1224 disabled auto-merge June 19, 2023 07:20
@hanabi1224 hanabi1224 enabled auto-merge (squash) June 19, 2023 07:21
@hanabi1224 hanabi1224 merged commit eb8e718 into main Jun 19, 2023
@hanabi1224 hanabi1224 deleted the hm/filecoin-default-cid-builder branch June 19, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LesnyRumcajs LesnyRumcajs LesnyRumcajs approved these changes

@lemmih lemmih lemmih approved these changes

@elmattic elmattic Awaiting requested review from elmattic

@jdjaustin jdjaustin Awaiting requested review from jdjaustin

@creativcoder creativcoder Awaiting requested review from creativcoder

@sudo-shashank sudo-shashank Awaiting requested review from sudo-shashank

@aatifsyed aatifsyed Awaiting requested review from aatifsyed

@ruseinov ruseinov Awaiting requested review from ruseinov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hanabi1224 @LesnyRumcajs @lemmih