Make OctetString constructor public

[jadamcrain]

fixes #370

[gemini-code-assist]

Summary of Changes

Hello @jadamcrain, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses an issue by making the OctetString constructor publicly accessible. This change enhances the usability and flexibility of the OctetString type, allowing it to be directly instantiated from external code, which is crucial for integration and testing scenarios.

Highlights

• OctetString Constructor: The constructor for OctetString has been made public by adding a full initializer during its build process.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request aims to make the OctetString struct constructible from the FFI layer by adding an initializer. While the intent is correct, the current implementation uses FunctionReturnStruct and an iterator field, which makes it non-constructible for FFI clients. My review provides a detailed comment on how to correctly implement this by using UniversalStruct and a collection type, which will require a slightly larger refactoring.

[gemini-code-assist]

The goal of making OctetString constructible is correct, but this implementation will not work as intended for FFI clients due to a couple of fundamental issues.

1. FunctionReturnStruct is not user-constructible: The octet_string_struct is defined as a FunctionReturnStruct. This struct type is designed for values returned from Rust and is opaque to FFI clients, meaning they cannot construct it. To create a user-constructible struct, you should use UniversalStruct.

2. Iterator as a constructor argument: The value field has the type byte_iterator. FFI clients (e.g., in C# or Java) cannot create and pass an iterator. They work with collections, like a byte array. The value field should be changed to use a CollectionHandle, such as the byte_collection already defined in this file.

To properly expose a constructible OctetString, a refactoring of build_octet_string is needed. Here is a suggested implementation:

fn build_octet_string(
    lib: &mut LibraryBuilder,
    byte_collection: &CollectionHandle,
) -> Result<
    (
        UniversalStructHandle,
        AbstractIteratorHandle,
        AbstractIteratorHandle,
    ),
    BindingError,
> {
    let byte_it = lib.define_iterator_with_lifetime("byte_iterator", Primitive::U8)?;

    let octet_string_struct_decl = lib.declare_universal_struct("octet_string")?;
    let octet_string_struct = lib
        .define_universal_struct(octet_string_struct_decl)?
        .add("index", Primitive::U16, "Point index")?
        .add("value", byte_collection.clone(), "Point value")?
        .doc("Octet String point")?
        .end_fields()?
        .add_full_initializer("init")?
        .build()?;

    let octet_string_iterator =
        lib.define_iterator_with_lifetime("octet_string_iterator", octet_string_struct.clone())?;

    Ok((octet_string_struct, octet_string_iterator, byte_it))
}

Please note that this change will require further adjustments:

• The define() function will need to be modified to pass byte_collection.
• The SharedDefinitions struct will need to be updated to use UniversalStructHandle for _octet_string.

I'm providing this broader context here as I'm limited to commenting on the changed lines.