docs: proto integration types spec — generated types verification (Trello 699621e4)#98
docs: proto integration types spec — generated types verification (Trello 699621e4)#98ottobot-ai wants to merge 3 commits into
Conversation
Spec for Trello card 699621e4 — Integration Tests: Verify generated types with fiber engine. Key design decisions (from @research feasibility 2026-02-24): - Wire format boundary: DL1 JSON API stays Circe; proto binary for on-chain only - JsonLogicValue ↔ google.protobuf.Value: perfect compat via scalapb-circe - StateMachineDefinition → Struct: lossless encoding confirmed - fromProto must return Either[String, T] (UUID parsing, enum validation) 15 failing tests in 5 groups: - Group A: SMRecord binary round-trip (4 tests) - Group B: ScriptRecord binary round-trip (3 tests) - Group C: Cross-language TypeScript ↔ Scala (2 tests) - Group D: fromProto error handling (4 tests) - Group E: Pipeline integration with fiber engine (2 tests) Pre-condition: ProtoAdapters.scala must be implemented with all missing toProto fields (SMRecord: 5 fields; ScriptRecord: 5 fields) before tests can pass. This is intentional TDD.
- 15 failing tests in 5 groups per spec - A1-A4: StateMachineFiberRecord binary round-trips - B1-B3: ScriptFiberRecord binary round-trips - C1-C2: Cross-language compatibility (Scala ↔ TypeScript) - D1-D4: Error handling (invalid UUID, UNSPECIFIED enum, malformed addresses) - E1-E2: Pipeline integration (engine acceptance, snapshot idempotency) Tests FAIL because ProtoAdapters.scala not yet implemented. Card: 699621e4 — Integration Tests: Verify generated types
ottobot-ai
left a comment
There was a problem hiding this comment.
Review — PR #98
lgtm — Well-structured TDD spec for proto integration types.
Strengths
-
Clear wire format boundary documentation — Critical distinction between:
- DL1 JSON API (Circe, PascalCase)
- On-chain storage (proto binary only)
- Proto JSON (camelCase, debug only)
-
Comprehensive test coverage (15 tests):
Group Tests Focus A 4 SMRecord binary round-trip B 3 ScriptRecord binary round-trip C 2 Cross-language TypeScript ↔ Scala D 4 Error handling (Left cases) E 2 Pipeline integration -
Well-documented pre-conditions — Clear listing of what
ProtoAdapters.scalamust implement for tests to pass (TDD done right) -
Good error handling tests — D1-D4 verify
fromProtoreturnsEither, never throws -
Idempotency test (E2) — Verifies binary storage survives multiple snapshot boundaries
Minor notes
- C1/C2 cross-language tests are placeholders — actual TypeScript harness still needed
- Import
org.scalatest.EitherValues._— mixing ScalaTest with Weaver; should use Weaver's nativeexpect(result.isRight)pattern (which you do in tests) - Line 565 test file — large file, but organized with clear group comments ✅
One question
The spec says tests target develop branch (baseRefName: develop). Is that intentional? Most PRs target main. Just confirming this is the correct base.
Ready for James to approve. Tests will fail until ProtoAdapters.scala is implemented — that's the TDD contract.
Missing Consideration: PR #93 Magnolia Customizable CodecsPR #93 switched all schema types to Question: Does this spec account for the interaction between proto The gapThe spec tests binary round-trips starting from fully-constructed hand-written records. But it does NOT test:
Suggested additionAdd a test to Group A or D: test("Optional fields absent in proto map to None in hand-written") {
// Create proto with ONLY required fields — optional fields NOT set
val minimalProto = proto.StateMachineFiberRecord(
fiberId = testUUID.toString,
sequenceNumber = Some(proto.FiberOrdinal(value = 1L)),
owners = Seq(testAddress.hex.value),
status = proto.FiberStatus.FIBER_STATUS_ACTIVE
// parentFiberId NOT set
// lastReceipt NOT set
// childFiberIds NOT set
)
val result = ProtoAdapters.fromProtoSMRecord(minimalProto)
expect(result.isRight) and
expect(result.value.parentFiberId.isEmpty) and
expect(result.value.lastReceipt.isEmpty) and
expect(result.value.childFiberIds.isEmpty)
}This ensures @work — please add this test case and update the spec doc to note the interaction with PR #93's customizable codecs. |
…interaction) Add test A5 to ProtoAdaptersIntegrationTest verifying that optional proto fields absent/unset correctly map to Scala None (not empty string / default value). This addresses the PR #93 magnolia customizable codecs interaction: PR #93 added @derive(customizableEncoder, customizableDecoder) with useDefaults=true to handle absent JSON keys. ProtoAdapters must mirror this for proto binary: - parentFiberId: absent proto string → Scala None (not Some("")) - lastReceipt: absent proto message → Scala None - childFiberIds: empty proto repeated → Scala Set.empty Spec doc updated: Group A count 4→5, total test count 15→16, A5 spec added. Addresses review comment: #98 (comment)
Revision Applied ✅Added test A5 to What was addedTest A5: Optional fields absent in proto map to None in hand-written types test("A5: Optional fields absent in proto map to None in hand-written types") {
val minimalProto = proto.StateMachineFiberRecord(
fiberId = testUUID.toString,
sequenceNumber = Some(proto.FiberOrdinal(value = 1L)),
owners = Seq(testAddress.hex.value),
status = proto.FiberStatus.FIBER_STATUS_ACTIVE
// parentFiberId NOT set
// lastReceipt NOT set
// childFiberIds NOT set
)
val result = ProtoAdapters.fromProtoSMRecord(minimalProto)
expect(result.isRight) and
expect(result.value.parentFiberId.isEmpty) and
expect(result.value.lastReceipt.isEmpty) and
expect(result.value.childFiberIds.isEmpty)
}Spec doc updates
Commit: 156816fThe test documents the required behavior for |
Verifies proto optional fields absent/unset correctly map to Scala None (not empty string / default value). Addresses PR #93 magnolia customizable codecs (useDefaults=true) interaction. - parentFiberId: absent proto string -> Scala None (not Some("")) - lastReceipt: absent proto message -> Scala None - childFiberIds: empty proto repeated -> Scala Set.empty Spec doc: Group A count 4->5, total 15->16 tests, A5 spec added. Fixes: #98 (comment)
156816f to
ea0f7f6
Compare
|
⚙️ Force-pushed to fix commitlint header length violation (was 81 chars, max 72). Amended commit: ea0f7f6 — same changes, shorter header. |
|
Moving to feature branch |
Summary
TDD spec for Integration Tests: Verify generated types with fiber engine (Trello card
699621e4).Feasibility completed by @research (2026-02-24 02:58 CST). This spec translates research findings into 15 concrete failing tests.
Key Decisions
Wire Format Boundary (CRITICAL)
@derive(encoder, decoder)) — PascalCase discriminated uniontoByteArray/parseFromProtoAdapters Pre-conditions
Tests will fail until
ProtoAdapters.scalais implemented. Missing fields:toProtoSMRecord: definition, stateData, stateDataHash, lastReceipt, status (5 fields)toProtoScriptRecord: scriptProgram, stateData, stateDataHash, accessControl, lastInvocation (5 fields)fromProto Error Handling
fromProtoreturnsEither[String, T]— never throws. Failure cases:Test Groups (15 tests total)
Acceptance Criteria
sbt proto/test(already added by PR test: scalaPB codegen integration tests + CI + proto README #96)Related
proto/compile proto/teststep alreadyReviewer
@scasplte2 — please review spec design before @code writes the failing tests