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

[Codegen] Generate Swift bindings #3122

Merged
merged 442 commits into from
May 11, 2023
Merged

[Codegen] Generate Swift bindings #3122

merged 442 commits into from
May 11, 2023

Conversation

lamafab
Copy link
Contributor

@lamafab lamafab commented Apr 26, 2023

EDIT: Please check the latest update: #3122 (comment)

About

This is the follow up to the codegen-v2 parser (#3065) migration which generates Swift bindings from the C headers. I also recommend to read the Design Decision section, respectively my Proposal at the end.

ToDo

  • Convert C header specification into datastructures easier to work with ("manifest").
  • Complete Swift bindings.
    • Write file template for Swift bindings.
      • Improve formatting, make it nicer.
    • Process manifest, handle markers accordingly.
    • Handle TWString and TWData types.
    • Generate bindings for all components:
      • Structs/classes
        • Add superclasses where appropriate.
      • Enums
        • Add superclasses where appropriate.
        • Add description field for string representation where required.
      • Enum extensions in separate file
      • Inits/constructors
      • Deinits/deconstuctors
      • Methods
      • Properties
      • Proto types
        • I don't fully understand this part yet, investigating
    • Replicate current Swift API perfectly, implement specific handlers where required.
      • WIP, handling all edge-cases can be tricky.
      • (2023-05-01) Build target with generated files and everything seems okay, all unit tests passed. Will do additional checks to verify this is the case
  • Cleanup code:
    • Proper error handling.
    • Document types and functions.

Execution

To generate the verbose C header grammar (to out/header_grammar.json):

$ cargo run -- parse-headers

To create the manifest (to out/manifest/):

$ cargo run -- create-manifest

To generate Swift bindings (to out/swift_bindings/):

$ cargo run -- generate-bindings --swift

Design Decision

First, let me introduce the two concepts C grammar and Manifest.

C grammar

The C grammar parser was introduced in #3065 and converts the C headers in include/ into a parsable JSON file where all the components and its properties are specified. The C grammar is very verbose and unnecessarily complex, given that the C headers are being (ab-)used with custom markers, typedefs, etc for workflow purposes.

An example of the C grammar:

See output
{
  "g_type": "function_decl",
  "value": {
    "name": "TWTransactionCompilerCompileWithSignatures",
    "params": [
      {
        "type": {
          "g_variant": "Mutable",
          "value": {
            "g_variant": "Enum",
            "value": "TWCoinType"
          }
        },
        "markers": [],
        "name": "coinType"
      },
      {
        "type": {
          "g_variant": "Mutable",
          "value": {
            "g_variant": "Pointer",
            "value": {
              "g_variant": "Unrecognized",
              "value": "TWData"
            }
          }
        },
        "markers": [
          {
            "g_variant": "NonNull"
          }
        ],
        "name": "txInputData"
      },
      {
        "type": {
          "g_variant": "Const",
          "value": {
            "g_variant": "Pointer",
            "value": {
              "g_variant": "Struct",
              "value": "TWDataVector"
            }
          }
        },
        "markers": [
          {
            "g_variant": "NonNull"
          }
        ],
        "name": "signatures"
      },
      {
        "type": {
          "g_variant": "Const",
          "value": {
            "g_variant": "Pointer",
            "value": {
              "g_variant": "Struct",
              "value": "TWDataVector"
            }
          }
        },
        "markers": [
          {
            "g_variant": "NonNull"
          }
        ],
        "name": "publicKeys"
      }
    ],
    "return_value": {
      "type": {
        "g_variant": "Mutable",
        "value": {
          "g_variant": "Pointer",
          "value": {
            "g_variant": "Unrecognized",
            "value": "TWData"
          }
        }
      },
      "markers": [
        {
          "g_variant": "NonNull"
        }
      ]
    },
    "markers": [
      {
        "g_variant": "TwExportStaticMethod"
      }
    ]
  }
}

Manifest

To make the C grammar easier to work with, we first convert it into a Manifest, which is a collection of simple structures. From there, each binding generator (Swift, Kotlin, etc) can then use that manifest for its own specific purposes with custom treatment/handlers, etc.

The manifest will play a more important role later on (if approved), as described in the Proposal section.

An example of the manifest structure:

See output
- name: TWTransactionCompilerCompileWithSignatures
  is_public: true
  is_static: true
  params:
  - name: coinType
    type:
      variant: enum
      value: TWCoinType
      is_constant: false
      is_nullable: false
      is_pointer: false
  - name: txInputData
    type:
      variant: void
      is_constant: true
      is_nullable: false
      is_pointer: true
      tags:
      - TW_DATA
  - name: signatures
    type:
      variant: struct
      value: TWDataVector
      is_constant: true
      is_nullable: false
      is_pointer: true
  - name: publicKeys
    type:
      variant: struct
      value: TWDataVector
      is_constant: true
      is_nullable: false
      is_pointer: true
  return_type:
    variant: void
    is_constant: true
    is_nullable: false
    is_pointer: true
    tags:
    - TW_DATA

Proposal: Deprecate C header & Parser

As explained in the C grammar concept section, the way we generate the C bindings introduces a lot of complexity. In the future - in my opinion - we should deprecate both the include/ directory and the C grammar parser entirely and only track and maintain the manifest.

The parser has now served its purpose to help generate the manifest and once we believe the manifest structure is a good fit, the parser should be retired. This would result in much less code, less maintenance and less complexity.

Feedback (or pushback) appreciated.

codegen-v2/src/codegen/swift/WalletCore.h Outdated Show resolved Hide resolved
codegen-v2/src/codegen/swift/functions.rs Show resolved Hide resolved
codegen-v2/src/codegen/swift/functions.rs Show resolved Hide resolved
codegen-v2/src/codegen/swift/functions.rs Show resolved Hide resolved
codegen-v2/src/codegen/swift/inits.rs Show resolved Hide resolved
codegen-v2/src/codegen/swift/mod.rs Show resolved Hide resolved
@lamafab
Copy link
Contributor Author

lamafab commented May 10, 2023

Okay, I was working on some changes (branch codegen-v2-swift-cleanup) but the changes introduced some issues I don't want to deal with right now given that we want to deploy today, especially this part:

        // Special handling: some functions do not follow standard camelCase
        // convention.
        #[rustfmt::skip]
        let pretty_name = if object.name() == "TWStoredKey" {
            pretty_name
                .replace("Json", "JSON")
                .replace("Hd", "HD")
        } else if object.name() == "TWPublicKey" {
            pretty_name
                .replace("Der", "DER")
        } else if object.name() == "TWHash" {
            pretty_name
                .replace("ripemd", "RIPEMD")
                .replace("Ripemd", "RIPEMD")
                .replace("sha512256", "sha512_256")
                .replace("sha3256", "sha3_256")
                .replace("sha256sha256", "sha256SHA256")
        } else if object.name() == "TWAES" {
            pretty_name
                .replace("Cbc", "CBC")
                .replace("Ctr", "CTR")
        } else {
            pretty_name
        };

I'm keeping this for now. @satoshiotomakan I'm putting your recommendations on the TODO list of my next PR. @Milerius I now added support for hex values in enums. I also noticed the following:

public enum HDVersion: UInt32, CaseIterable {
    case `none` = 0
    case `xpub` = 0x0488b21e
    case `xprv` = 0x0488ade4
    case `ypub` = 0x049d7cb2
    case `yprv` = 0x049d7878
    case `zpub` = 0x04b24746
    case `zprv` = 0x04b2430c
    case `ltub` = 0x019da462
    case `ltpv` = 0x019d9cfe
    case `mtub` = 0x01b26ef6
    case `mtpv` = 0x01b26792
    case `dpub` = 0x2fda926
    case `dprv` = 0x2fda4e8
    case `dgub` = 0x02facafd
    case `dgpv` = 0x02fac398
}

The values for dpub and dprv seem to have a missing character. I'm not sure if that's intentional, I'm just keeping it the same for now.

Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

LGTM!
I have only non-blocking comments

codegen-v2/src/codegen/swift/templates/WalletCore.h Outdated Show resolved Hide resolved
codegen-v2/src/lib.rs Outdated Show resolved Hide resolved
codegen-v2/src/main.rs Outdated Show resolved Hide resolved
swift/Podfile.lock Outdated Show resolved Hide resolved
tools/generate-files Show resolved Hide resolved
codegen-v2/src/tests/samples/class.input.yaml Outdated Show resolved Hide resolved
@lamafab
Copy link
Contributor Author

lamafab commented May 10, 2023

Thanks for the feedback, @satoshiotomakan. I added some of your recommended changes as part of 5a74076. Also, see issue #3159

Copy link
Collaborator

@satoshiotomakan satoshiotomakan left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! Re-approve

@lamafab lamafab merged commit 0f70467 into master May 11, 2023
@lamafab lamafab deleted the codegen-v2-swift branch May 11, 2023 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants