Skip to content

Conversation

@gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Dec 29, 2025

Description

Update tinyfield code generation using latest gnark-crypto. Sister PR: Consensys/gnark-crypto#781


Note

Modernizes code generation and refreshes generated artifacts.

  • Switches field codegen to new github.com/consensys/gnark-crypto/field/generator.Generate API; removes config.FieldDependency usage and flattens gkrConfig
  • Regenerates internal/smallfields/tinyfield (doc tweaks, adds SetBytesCanonical property test) and GKR backends across curves
  • Bumps Go to 1.24.9 and updates deps (gnark-crypto, compress, bitset, pprof, x/{crypto,exp,sync,sys}); updates go.sum
  • Refreshes copyright headers to 2025-2026 across generated files

Written by Cursor Bugbot for commit 56b74fc. This will update automatically on new commits. Configure here.

Copilot AI review requested due to automatic review settings December 29, 2025 17:51
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the tinyfield code generation to use the latest gnark-crypto API. The changes migrate from the deprecated GenerateFF API to the new simpler generator.Generate function, and replace the structured FieldDependency configuration with direct field assignments in the gkrConfig struct.

  • Migrated field generator API from GenerateFF with config.NewFieldConfig to the new generator.Generate function
  • Refactored gkrConfig struct to use direct field assignments instead of embedded FieldDependency struct
  • Added test coverage for the new SetBytesCanonical method generated by the updated gnark-crypto

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

File Description
internal/smallfields/tinyfield/element_test.go Added property-based test for SetBytesCanonical method to verify round-trip consistency with Bytes()
internal/smallfields/tinyfield/doc.go Updated documentation to remove specific mention of AVX512/NEON instructions, making it more generic
internal/generator/backend/main.go Migrated to new gnark-crypto generator API, replacing GenerateFF with Generate and flattening gkrConfig struct fields

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +131 to +137
// conf, err := config.NewFieldConfig(d.Curve, "Element", d.AutoGenerateField, false)
// if err != nil {
// panic(err)
// }
// if err := generator.GenerateFF(conf, d.RootPath, generator.WithASM(nil)); err != nil {
// panic(err)
// }
Copy link

Copilot AI Dec 29, 2025

Choose a reason for hiding this comment

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

The commented-out code should be removed rather than left in place. This old API code has been replaced by the new generator.Generate call and is no longer needed. Keeping dead code in the codebase reduces maintainability and can cause confusion for future developers.

Suggested change
// conf, err := config.NewFieldConfig(d.Curve, "Element", d.AutoGenerateField, false)
// if err != nil {
// panic(err)
// }
// if err := generator.GenerateFF(conf, d.RootPath, generator.WithASM(nil)); err != nil {
// panic(err)
// }

Copilot uses AI. Check for mistakes.
// }
// if err := generator.GenerateFF(conf, d.RootPath, generator.WithASM(nil)); err != nil {
// panic(err)
// }
Copy link

Choose a reason for hiding this comment

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

Commented-out old implementation code left in codebase

The old implementation using config.NewFieldConfig and generator.GenerateFF with generator.WithASM(nil) has been left as commented-out code after the migration to the new generator.Generate API. This dead code adds noise to the codebase and could confuse future maintainers. It appears to have been kept for reference during development but wasn't cleaned up before the PR.

Fix in Cursor Fix in Web

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.

2 participants