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

[skip changelog] chore: types: remove all references to FFI/rust implementations of CommP/sha2-256-254 #12345

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

ribasushi
Copy link
Collaborator

No description provided.

@ribasushi ribasushi force-pushed the nonffi_harder branch 2 times, most recently from 7a85ec6 to 16ec73d Compare August 5, 2024 22:10
@ribasushi ribasushi marked this pull request as ready for review August 5, 2024 22:19
@ribasushi
Copy link
Collaborator Author

@rvagg this is an update to your #11581 branch, building on your work in commp-utils, together with filecoin-project/go-commp-utils#18

What you need to review is the last 3 commits above only, as per
image

@rvagg
Copy link
Member

rvagg commented Aug 6, 2024

🥳 🎆 🥳

1 2 3 4 5

🥳 🎆 🥳

Congratulations! You win ... something

@rvagg rvagg force-pushed the rvagg/non-storage-verifier branch from f3019f3 to b56788f Compare August 6, 2024 08:57
@rvagg
Copy link
Member

rvagg commented Aug 6, 2024

@ribasushi would you mind rebasing on rvagg/non-storage-verifier please, it's a bit hard to review this as you pulled in some master commits. I've rebased mine to current master but now we have overlapping commits.

@rvagg
Copy link
Member

rvagg commented Aug 6, 2024

fwiw I don't find per-commit reviewing very helpful or productive and would rather a changeset be complete enough to review as a body

@ribasushi ribasushi changed the title Nonffi harder [skip changelog] Nonffi harder Aug 6, 2024
@rvagg
Copy link
Member

rvagg commented Aug 6, 2024

this seems ok to me but goes deeper than I was doing in and touches storage/ stuff, so I'd like @magik6k's sign-off on this, and hopefully #11581 too would be great so we can get this flattened and merged.

@rjan90
Copy link
Contributor

rjan90 commented Aug 7, 2024

Could you update the PR title to match the: <type>: <area>: <subject> format?

Types: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test, chore
Area: e.g. api, chain, state, mempool, multisig, networking, paych, proving, sealing, wallet, deps

@ribasushi
Copy link
Collaborator Author

@rjan90 this PR is not against master, it's an internal to-be-flattened-into-another one.

Buit feel free to edit if still important.

@rvagg
Copy link
Member

rvagg commented Aug 8, 2024

I think it's going to have to be against master eventually, I'm having a hard enough time getting reviews on #11581 and adding this to the mix is going to narrow down the number of potential reviewers even further so I'm inclined to tackle merging #11581 first and dealing with this second.

@ribasushi ribasushi changed the base branch from rvagg/non-storage-verifier to master August 8, 2024 17:17
@ribasushi ribasushi changed the title [skip changelog] Nonffi harder [skip changelog] chore: types: remove all references to FFI/rust implementations of CommP/sha2-256-254 Aug 8, 2024
@ribasushi
Copy link
Collaborator Author

think it's going to have to be against master eventually

Changed references/title. This PR is a single commit over #11581 as of writing of this note

image image

@rvagg @masih can I have a ✅ review on this please.

@rvagg
Copy link
Member

rvagg commented Aug 9, 2024

Sorry, I squashed my noisy commits in #11581 when merging, then I put #12358 on top of it and it got merged. So now this is against master but has conflicts and shows the changes in #11581 again. Sorry to mess you around on this @ribasushi, but I should be able to review this now and merge since it's not my PR. I just need to do some double-checking of the logic in here which is a bit more complicated than #11581 was.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

I'm OK with this but don't want to merge it unilaterally. I'd really like a @filecoin-project/curio person to cast an eye over it since it touches storage/ pieces

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Stared at this for a while and see no reason why it shouldn't work. If something doesn't work we'll notice rather quickly

@rvagg rvagg merged commit 0b94fee into filecoin-project:master Aug 13, 2024
82 checks passed
@ribasushi ribasushi deleted the nonffi_harder branch August 13, 2024 11:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants