Skip to content

refactor(fs): return named structs from fold APIs - #251

Closed
junbyjun1238 wants to merge 1 commit into
ethereum:devfrom
junbyjun1238:fold-output-structs-dev
Closed

refactor(fs): return named structs from fold APIs#251
junbyjun1238 wants to merge 1 commit into
ethereum:devfrom
junbyjun1238:fold-output-structs-dev

Conversation

@junbyjun1238

Copy link
Copy Markdown

This PR replaces tuple-returning fold outputs with named structs and updates the relevant CycleFold call sites to use the new named results.

This is a breaking API cleanup that makes fold artifacts explicit by name across the folding-scheme interfaces.

Note to reviewers: Because this replaces the signatures of the canonical folding traits, the migration necessarily updates the current fs implementations (Nova, HyperNova, Mova, Ova, and ProtoGalaxy) and the downstream CycleFold consumers. Most of the resulting churn is mechanical.

Why:

  • The CycleFold path still passes large positional tuples like (RW, RU, Proof, Challenge) and (RU, Challenge) through some of its most complex code paths.
  • Naming those artifacts makes these call sites easier to read and review.

What changes:

  • change FoldingSchemeProver::prove(...) to return FoldStep
  • change FoldingSchemePartialVerifierGadget::verify_hinted(...) to return PartialVerifierStep
  • migrate the existing Nova, HyperNova, Mova, Ova, and ProtoGalaxy implementations to the new return types
  • update the relevant CycleFold call sites to consume the new named structs

Testing:

  • cargo test -p sonobe-fs
  • cargo test -p sonobe-ivc

References:

@winderica winderica self-assigned this Apr 28, 2026
@winderica winderica added this to the Stabilize library milestone May 24, 2026
Comment on lines +11 to +20
pub struct FoldStep<FS: FoldingSchemeDef + ?Sized, const M: usize, const N: usize> {
/// The next running witness after folding.
pub next_running_witness: FS::RW,
/// The next running instance after folding.
pub next_running_instance: FS::RU,
/// The proof artifact emitted by the folding step.
pub proof: FS::Proof<M, N>,
/// The challenge derived during the folding step.
pub challenge: FS::Challenge,
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While I think your idea of replacing it with a struct FoldStep is very nice, I have some (nitpicky) concern about the semantics of the naming of its fields in different contexts.

The current naming makes perfect sense in IVC, where there is only a single chain of execution, so "next" is clearly defined here. But for other use cases this may not be very suitable, for example, when a blinding folding scheme is used to hide witness w, we generate (W', U') by folding (w, u) into a random satisfying (W, U) pair, where the resulting (W', U') is essentially a randomized version of (w, u), but not the "next" witness-instance pair.

Do you think you can use a typed alias instead, or replace "next" with "folded" or something similar?

@winderica winderica left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @junbyjun1238, thank you a lot for your contribution and super sorry for the very late response!

I fully agree with you that having complex returning types is not a very elegant idea, and I have been thinking about solutions that can improve this ultimately, which leads to this PR.

Specifically, Challenges are now eliminated, thanks to the introduction of RecordingTranscript and ReplayTranscript, which allow us to save and replay challenges without passing them throughout the protocols: now (RW, RU, Proof, Challenge) becomes (RW, RU, Proof), and (RU, Challenge) becomes simply RU.

Since #261 will be merged to the staging branch, could you target staging instead of dev?

@junbyjun1238

Copy link
Copy Markdown
Author

Thanks a lot for the detailed review.

I agree that folded_* would be a better naming direction than next_*. I also looked at #261, and it seems to change the API shape enough that this PR should not be carried forward as-is.

I'll close this one for now, and revisit the remaining tuple-return shape on top of staging after #261 lands, if it still looks useful then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants