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

[NayNay] User Programs: Add Program + testing #173

Merged
merged 4 commits into from
Jul 16, 2024
Merged

Conversation

rh0delta
Copy link
Contributor

@rh0delta rh0delta commented Jul 15, 2024

Related Issue(s)

Proposed Changes

  • updated folder structure for consistency
  • added new method for adding programs as a pure function
  • added tests

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed

Screenshots (if applicable)

Additional Context

Checklist

  • I have performed a self-review of my code.
  • I have added tests.
  • I have commented my code.
  • I have included a CHANGELOG.md entry.
  • I have updated documentation in github.com:entropyxyz/entropy-docs, where necessary.

rh0delta added 4 commits July 15, 2024 16:21
- updated folder structure for consistency
- added new method for adding programs as a pure function
- added tests
@rh0delta rh0delta changed the base branch from main to dev July 16, 2024 02:37
@mixmix
Copy link
Contributor

mixmix commented Jul 16, 2024

Things missing from checklist @rh0delta
I'll review anyway

Comment on lines +4 to +12
export async function addProgram (entropy: Entropy, { programPointer, programConfig, verifyingKey }: AddProgramParams): Promise<void> {
return entropy.programs.add(
{
program_pointer: programPointer,
program_config: programConfig,
},
verifyingKey
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ this extraction doesn't seem to add much other than a variant API.
It's fine if it helps parts of the code conform to the same pattern and/or we expect the complexity here to grow? Feels like marginal gain because it doesn't really improve readability and the function you're wrapping is an SDK method which is already tested.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your choice as to whether it's worth the change

Copy link
Contributor

Choose a reason for hiding this comment

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

if we're going single-function-files then I'd probably call the file add-program.ts
I wouldn't worry because I think we're gonna workshop the file/folder structure of this repo in the "near" future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the idea here is to conform to a structure where we are essentially creating these pure functions to work with both the tui and programmatic cli, mainly to reduce repetition of the same code in multiple files.

@rh0delta rh0delta merged commit ff30a2c into dev Jul 16, 2024
2 checks passed
@rh0delta rh0delta deleted the naynay/add-program branch July 16, 2024 16:30
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.

add a program
2 participants