Skip to content

fix(generate): send present but falsy optional values as present - #197

Open
marc0olo wants to merge 2 commits into
mainfrom
fix/optional-presence-encoding
Open

marc0olo wants to merge 2 commits into
mainfrom
fix/optional-presence-encoding

Conversation

@marc0olo

@marc0olo marc0olo commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Bottom of a two-PR stack (#198 builds on it) fixing conversions that are silently wrong.

Behaviour

This changes what goes on the wire. A canister that received nothing for these values now receives what the caller asked to send.

you write you mean before after
record field { count: 0n } (opt nat) present, value 0 [], absent [0n]
record field { flag: false }, { name: "" } present [] [false], [""]
record field omitted or undefined absent [] []
variant payload { __kind__: "count", count: 0n } present, value 0 { count: [] } { count: [0n] }
variant payload count: null absent { count: [] } { count: [] }
bare opt argument 0n present [0n] [0n], unchanged

Why

Optional record fields and variant payloads were encoded with a truthiness test (value.count ? candid_some(…) : candid_none()), so 0, false and "" could not be sent at all. The generated code was valid and typechecked, which is why snapshots and typechecks passed over it. The test is now != null; bare opt arguments already compared with null.

Testing

tests/conversion.test.ts generates the bindings, calls the generated wrapper with a stub actor and checks what reaches it. It fails on main with expected { count: [] } to deeply equal { count: [ 0n ] }. tests/utils/wrapper.ts loads the wrapper against a minimal @icp-sdk/core stub. The snapshot changes are the example fixture's two optional fields.

🤖 Generated with Claude Code

Optional record fields and variant payloads were encoded with a
truthiness test, so 0, false and "" were sent as absent. Test presence
with `!= null` instead.

The new conversion test runs the generated wrapper against a stub actor
and checks what it sends.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Repeated wrapper calls overwrite cleanup state and leak temporary directories.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Corrects TypeScript-to-Candid conversion so falsy optional values remain present.

Changes:

  • Replaces truthiness checks with nullish presence checks.
  • Adds runtime conversion tests and supporting fixtures.
  • Updates generated snapshots.
File Description
src/​core/​generate/​rs/​src/​bindings/​typescript_native/​conversion_functions_generator.rs Generates nullish optional checks.
tests/​conversion.test.ts Tests optional-value conversion.
tests/​utils/​wrapper.ts Loads generated wrappers with stub dependencies.
tests/​assets/​optional_presence.did Defines optional conversion fixtures.
tests/​snapshots/​generate/​example/​example.ts.snapshot Updates generated output.
tests/​snapshots/​wasm-generate/​no-root-export/​example/​example.ts.snapshot Updates no-root-export output.
tests/​snapshots/​wasm-generate/​root-export/​example/​example.ts.snapshot Updates root-export output.
tests/​snapshots/​wasm-generate/​typescript/​example/​example.ts.snapshot Updates TypeScript output.
tests/​snapshots/​wasm-generate/​typescript-root-export/​example/​example.ts.snapshot Updates TypeScript root-export output.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/conversion.test.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The focused generator fix correctly preserves falsy values and is covered by runtime tests and snapshots.

Review effort: Balanced
Findings: None

Resolved since last review (1)

This branch has not been deployed

No deployments
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