Skip to content

✨ Enable ssz storage for prover inputs #72

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

swetshaw
Copy link
Contributor

πŸ“ Description

Enables storage of prover inputs in SSZ format

βœ… Solved Issues

Resolves KKRT-104

πŸ”„ Change

  • ✨ New feature (e.g. API route, major perf improvement...)
  • πŸ› Bug fix
  • 🧹 Chore (e.g. package upgrades, configuration change, refactor, small perf improvement, typos...)
  • πŸ§ͺ Tests
  • 🏭 DevOps (e.g. CI-CD, development environment upgrade, security upgrade, automation addition...)
  • πŸ“š Documentation
  • πŸ’₯ Breaking Change

πŸ’₯ Breaking Change (if applicable)

πŸ“‹ Checklist

  • I have added tests to cover my changes, if applicable.
  • I have updated relevant documentation for my changes.
  • I have included references to issues resolved by this Pull Request
  • I have set a Pull Request title that respects gitmoji
  • I have set a Pull Request's label to one of type.feat,type.fix,type.chore,type.test,type.docs,type.devops
  • I have set a Pull Request's label to one of breaking-change or non-breaking-change
  • I have documented breaking changes and migration path, if applicable

πŸ““ Additional Notes

@swetshaw swetshaw added type.feat PR for a new feature non-breaking-change PR did not introduce any breaking change labels Feb 24, 2025
@swetshaw swetshaw requested a review from nmvalera as a code owner February 24, 2025 07:01
@nmvalera nmvalera marked this pull request as draft March 25, 2025 22:51
@nmvalera nmvalera requested a review from Copilot March 28, 2025 10:33
Copy link

@Copilot 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 pull request enables the storage of prover inputs in SSZ format, as demonstrated by the new test that validates conversion between JSON and SSZ representations.

  • Adds a new test file to verify the conversion of prover inputs to SSZ and back to JSON.
  • Incorporates debugging code to output JSON files on failure for further inspection.
Files not reviewed (1)
  • go.mod: Language not supported
Comments suppressed due to low confidence (1)

src/prover-input/ssz/input_test.go:55

  • [nitpick] Consider adding a call to t.Helper() at the beginning of this helper function to improve error reporting clarity in test failures.
func LoadProverInputJSON(t *testing.T, path string) *input.ProverInput {

Comment on lines +44 to +50
// store the original and final json in a file
// this is for debugging purposes
// TODO: remove this after fixing the issue
// issue: the uncles and transactions field is not being serialized correctly
// check the original.json and final.json files for more details
os.WriteFile("original.json", originalJSON, 0644)
os.WriteFile("final.json", jsonData, 0644)
Copy link
Preview

Copilot AI Mar 28, 2025

Choose a reason for hiding this comment

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

Debugging file write statements are left in the test; consider removing them or adding a flag to prevent unwanted side effects during automated test runs.

Suggested change
// store the original and final json in a file
// this is for debugging purposes
// TODO: remove this after fixing the issue
// issue: the uncles and transactions field is not being serialized correctly
// check the original.json and final.json files for more details
os.WriteFile("original.json", originalJSON, 0644)
os.WriteFile("final.json", jsonData, 0644)
if debugMode {
// store the original and final json in a file
// this is for debugging purposes
// TODO: remove this after fixing the issue
// issue: the uncles and transactions field is not being serialized correctly
// check the original.json and final.json files for more details
os.WriteFile("original.json", originalJSON, 0644)
os.WriteFile("final.json", jsonData, 0644)
}

Copilot uses AI. Check for mistakes.

Comment on lines +49 to +50
os.WriteFile("original.json", originalJSON, 0644)
os.WriteFile("final.json", jsonData, 0644)
Copy link
Preview

Copilot AI Mar 28, 2025

Choose a reason for hiding this comment

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

The error return from os.WriteFile is not checked; consider handling this error to ensure that debugging outputs are written as expected.

Suggested change
os.WriteFile("original.json", originalJSON, 0644)
os.WriteFile("final.json", jsonData, 0644)
if err := os.WriteFile("original.json", originalJSON, 0644); err != nil {
t.Fatalf("Failed to write original JSON to file: %v", err)
}
if err := os.WriteFile("final.json", jsonData, 0644); err != nil {
t.Fatalf("Failed to write final JSON to file: %v", err)
}

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking-change PR did not introduce any breaking change type.feat PR for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants