Skip to content

feat: add ProgramBuilder and InstanceBuilder for creating PROGRAM and INSTANCE messages#91

Merged
odesenfans merged 1 commit into
mainfrom
feat/program-instance-builders
Mar 18, 2026
Merged

feat: add ProgramBuilder and InstanceBuilder for creating PROGRAM and INSTANCE messages#91
odesenfans merged 1 commit into
mainfrom
feat/program-instance-builders

Conversation

@odesenfans
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown

@foxpatch-aleph foxpatch-aleph left a comment

Choose a reason for hiding this comment

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

The PR adds well-structured ProgramBuilder and InstanceBuilder following the existing builder pattern. The code is clean, tests are comprehensive for defaults and options, and the API exports are properly updated. However, there's one potential oversight: ProgramBuilder has an http field with default true but no setter method to disable it, which may be unintentional.

crates/aleph-sdk/src/messages.rs (line 214): The http field in ProgramBuilder is set to true by default (line 257) but there's no http() setter method to change it. If this is intentional (HTTP triggers always enabled), consider adding a comment. If users should be able to disable HTTP triggers, add a setter method similar to persistent().

crates/aleph-sdk/src/messages.rs (line 892): The test test_program_builder_defaults verifies http: true is set correctly, but test_program_builder_with_options could also include a test case for setting http: false if that setter is added.

crates/aleph-sdk/src/messages.rs (line 636): Minor: In InstanceBuilder::build(), reproducible: false and shared_cache: false are hardcoded. Consider whether these should be configurable or if the comment should explain why they're fixed for instances.

@odesenfans odesenfans force-pushed the feat/program-instance-builders branch from 964bf05 to 996f2d4 Compare March 18, 2026 13:30
Copy link
Copy Markdown

@foxpatch-aleph foxpatch-aleph left a comment

Choose a reason for hiding this comment

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

This PR adds well-designed ProgramBuilder and InstanceBuilder following the existing builder pattern used by PostBuilder, AggregateBuilder, and ForgetBuilder. The implementation is correct, with sensible defaults (1 vCPU, 128 MiB, 1 second timeout). Tests cover both default behavior and customized options for both builders. The code is clean and follows project conventions. Minor suggestions: consider renaming InstanceBuilder::ssh_keys() to authorized_keys() for consistency with ProgramBuilder, and adding doc comments to public methods.

crates/aleph-sdk/src/messages.rs (line 593): Method naming inconsistency: InstanceBuilder::ssh_keys() vs ProgramBuilder::authorized_keys() (line 373). Both set the same field (authorized_keys). Consider renaming to authorized_keys() for API consistency between the two builders.

crates/aleph-sdk/src/messages.rs (line 196): Consider adding doc comments to ProgramBuilder and its methods. While consistent with existing builders (which also lack docs), documentation would help users understand the purpose and defaults of each configuration option.

crates/aleph-sdk/src/messages.rs (line 466): Same as above - consider adding doc comments to InstanceBuilder struct and its methods.

crates/aleph-sdk/src/messages.rs (line 641): The comment about legacy fields is helpful. Consider also noting that reproducible and shared_cache are function-specific concepts that don't apply to instances.

@odesenfans odesenfans force-pushed the feat/program-instance-builders branch from 996f2d4 to abb70b2 Compare March 18, 2026 17:09
@odesenfans odesenfans merged commit 1c291c8 into main Mar 18, 2026
9 checks passed
@odesenfans odesenfans deleted the feat/program-instance-builders branch March 18, 2026 17:18
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