Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the “receipt” concept from the certification request/response flow in the SDK, updating the public client API, request CBOR encoding, response JSON shape, and associated unit tests.
Changes:
- Remove the
receiptparameter fromsubmitCertificationRequestand update callers. - Remove receipt handling/verification from
CertificationResponseJSON model and tests. - Remove receipt flag from
CertificationRequestCBOR encoding and update request encoding tests.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/api/CertificationResponseTest.ts | Removes receipt-related assertions/tests for CertificationResponse JSON round-tripping. |
| tests/unit/api/CertificationRequestTest.ts | Updates expected CBOR encoding to match the new request shape (no receipt flag). |
| src/api/IAggregatorClient.ts | Updates the public interface to remove the receipt parameter. |
| src/api/CertificationResponse.ts | Removes receipt structures and logic; response JSON now only includes status. |
| src/api/CertificationRequest.ts | Removes receipt flag from the request model and CBOR encoding. |
| src/api/AggregatorClient.ts | Updates request construction to no longer pass a receipt flag. |
| src/StateTransitionClient.ts | Updates wrapper to call the updated client method signature. |
| package.json | Bumps lint/tooling devDependency versions. |
| package-lock.json | Updates lockfile to reflect devDependency bumps and transitive changes. |
Comments suppressed due to low confidence (1)
src/api/CertificationResponse.ts:56
- JSDoc for
fromJSONis malformed and inaccurate: it declares@returns {Promise<>CertificationResponse>}even thoughfromJSONis synchronous and returnsCertificationResponse. Please update the return annotation (and remove the stray<>) so the generated typings/docs are correct.
/**
* Parse a JSON response object.
*
* @param {unknown} data Raw response
* @returns {Promise<>CertificationResponse>} Parsed response
* @throws {InvalidJsonStructureError} Error if the data does not match the expected shape
*/
public static fromJSON(data: unknown): CertificationResponse {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Submit a transaction commitment for inclusion in the ledger. | ||
| * | ||
| * @param {CertificationData} certificationData The certification data to submit | ||
| * @param {boolean} receipt Require a signed receipt of the commitment, default is false | ||
| * @returns Result status from the aggregator | ||
| */ | ||
| submitCertificationRequest(certificationData: CertificationData, receipt: boolean): Promise<CertificationResponse>; | ||
| submitCertificationRequest(certificationData: CertificationData): Promise<CertificationResponse>; |
There was a problem hiding this comment.
This interface change removes the receipt parameter from submitCertificationRequest, which is a breaking API change for SDK consumers. If this is intentional, consider documenting the breaking change in the public API docs/release notes (or providing a short-lived overload/deprecation path) so downstream callers can migrate cleanly.
| it('should encode and decode JSON to exactly same object', () => { | ||
| let response = CertificationResponse.fromJSON(CertificationResponse.create(CertificationStatus.SUCCESS).toJSON()); | ||
|
|
||
| expect(response.status).toBe(CertificationStatus.SUCCESS); | ||
| expect(response.receipt).toBeNull(); | ||
|
|
||
| // Test error response without receipt | ||
| response = CertificationResponse.fromJSON( | ||
| CertificationResponse.create(CertificationStatus.INVALID_PUBLIC_KEY_FORMAT).toJSON(), | ||
| ); | ||
|
|
||
| expect(response.status).toBe(CertificationStatus.INVALID_PUBLIC_KEY_FORMAT); | ||
| expect(response.receipt).toBeNull(); | ||
|
|
||
| // Test response with all fields | ||
| const signingService = new SigningService( | ||
| new Uint8Array(HexConverter.decode('0000000000000000000000000000000000000000000000000000000000000001')), | ||
| ); | ||
|
|
||
| response = await CertificationResponse.createWithReceipt( | ||
| signingService, | ||
| CertificationData.fromCBOR( | ||
| CborSerializer.encodeArray( | ||
| HexConverter.decode('8301410158210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798'), | ||
| CborSerializer.encodeByteString(new Uint8Array(32)), | ||
| CborSerializer.encodeByteString(new Uint8Array(32)), | ||
| CborSerializer.encodeByteString( | ||
| HexConverter.decode( | ||
| '8c3f91708445bf0ddec220f0821461bcf84860a8769275f9930e798d1f645d157bb6a2998c61941108b0993c5aed6a7b92ccf31d11b50fe80d9ff93da392336a01', | ||
| ), | ||
| ), | ||
| ), | ||
| ), | ||
| CertificationStatus.SUCCESS, | ||
| ); | ||
|
|
||
| const signature = response.receipt?.signature; | ||
|
|
||
| response = CertificationResponse.fromJSON(response.toJSON()); | ||
|
|
||
| expect(response.toJSON()).toEqual({ | ||
| receipt: { | ||
| publicKey: HexConverter.encode(signingService.publicKey), | ||
| signature: signature?.toJSON(), | ||
| }, | ||
| status: CertificationStatus.SUCCESS, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test name says it encodes/decodes JSON to the exact same object, but the assertions only check status. Since the response now only contains status, consider asserting that response.toJSON() matches the original JSON to make the test actually verify round-trip integrity (or rename the test to match what it checks).
| @@ -5,7 +5,7 @@ import { HexConverter } from '../../../src/serialization/HexConverter.js'; | |||
|
|
|||
| describe('CertificationRequest', () => { | |||
| it('should encode and decode CBOR to exactly same object', async () => { | |||
There was a problem hiding this comment.
The test description mentions "encode and decode" CBOR, but this test only asserts the encoded output (toCBOR()). Consider either adding a decode/parse assertion (if/when a decoder exists) or renaming the test to reflect that it is only checking encoding.
| it('should encode and decode CBOR to exactly same object', async () => { | |
| it('should encode CBOR to the expected bytes', async () => { |
No description provided.