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.

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

v3 Hamt update #982

Merged
merged 12 commits into from
Feb 5, 2021
Merged

v3 Hamt update #982

merged 12 commits into from
Feb 5, 2021

Conversation

austinabell
Copy link
Contributor

Summary of changes
Changes introduced in this pull request:

  • Updates pointer serialization format, all caching updates had been done previously (!go-interop flag)

Tested against https://github.com/austinabell/go-hamt-ipld/tree/rust/testing

Will wait on #981 and add another method to the API before opening PR for review

Reference issue to close (if applicable)

Closes #959

Other information and links

@austinabell austinabell added Status: Needs Review Spec Change Updates to implementation required due to a Filecoin spec change after implementation labels Feb 3, 2021
@austinabell austinabell marked this pull request as ready for review February 4, 2021 16:16
} else {
// Can't overwrite, return None and false that the Node was not modified.
return Ok((None, false));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@austinabell Isn't this where you were going to address that additional case you mentioned on our review call? Also, can you describe why that came to mind, so I can understand better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the point I was thinking through is not relevant to consensus, because the go implementation does not return the previous value. However, thinking through it a third time, I think that in this case a few lines above should be modified a bit.

The case is on set (overwrite = true) and the values are equal, should the value being passed in be returned back. Although it seems weird internally to do this, it would match a HashMap functionality by returning the value that existed. The difference would be that we are not swapping the values, and assuming PartialEq is implemented correctly. I am going to switch it to returning the value that existed, to match a HashMap implementation, but open to just returning the value passed in. This operation should be super quick in every case so I think it's the best decision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, just to be clear, the only case which this would have unintended behaviour is when PartialEq doesn't match serialized bytes equality AND this was the only value changed, the parent roots won't be updated. Definitely nothing to worry about, but worth keeping in mind that PartialEq must match bytes equality to interop in every case with go.

@austinabell austinabell requested a review from ec2 February 5, 2021 13:13
@austinabell austinabell merged commit 46f7bf6 into main Feb 5, 2021
@austinabell austinabell deleted the austin/v3/hamt branch February 5, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Spec Change Updates to implementation required due to a Filecoin spec change after implementation Status: Needs Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v3 Amt and Hamt update
3 participants