feat: add ProgramBuilder and InstanceBuilder for creating PROGRAM and INSTANCE messages#91
Conversation
foxpatch-aleph
left a comment
There was a problem hiding this comment.
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.
964bf05 to
996f2d4
Compare
foxpatch-aleph
left a comment
There was a problem hiding this comment.
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.
… INSTANCE messages
996f2d4 to
abb70b2
Compare
No description provided.