Recent small spec changes #213

diehuxx · 2024-03-26T05:40:53Z

Implement spec changes from this PR: TBD54566975/tbdex#269

There are currently no up-to-date test vectors for RFQ and Offerings. We can't create functioning test vectors until 2 tbdex SDKs have implemented these changes. I would prefer to take the following approach:

Merge these changes so this PR does not remain pending
Implement or have another engineer implement these changes in JS and KT
Regenerate and validate the test vectors in question
Open a new PR against tbdex-js to add the vectors.

Commits

issue	tbdex commits	tbdex-js commits
260	f55c588, 752aa4f	87c4162
261	9dc5148	1d82302
262	ff03448	85636b3
263	0614cf5	80da7b
253	5c1e67a	fceb7aca

changeset-bot · 2024-03-26T05:40:56Z

⚠️ No Changeset found

Latest commit: 7540d80

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

diehuxx · 2024-03-26T05:42:26Z

packages/protocol/build/compile-validators.js

When I updated the tbdex submodule to bring in the changes, I got a warning when running tbdex-js tests that compiled-validators.js was being read as ES and that was causing error. It fixed when I switched this to cjs. Seeking feedback if this is not the right thing to do here.

@frankhinek would know for sure, but I think if the is the only place it could be a problem and not work in certain environments ?

codecov-commenter · 2024-03-26T05:42:45Z

Codecov Report

Merging #213 (7540d80) into main (74b6ade) will decrease coverage by 0.58%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
- Coverage   93.53%   92.96%   -0.58%     
==========================================
  Files          39       39              
  Lines        3141     3142       +1     
  Branches      356      355       -1     
==========================================
- Hits         2938     2921      -17     
- Misses        203      221      +18

Components	Coverage Δ
protocol	`93.21% <100.00%> (-1.06%)`	⬇️
http-client	`94.83% <ø> (ø)`
http-server	`91.05% <100.00%> (ø)`

github-actions · 2024-03-26T05:42:57Z

TBDocs Report

✅ No errors or warnings

@tbdex/protocol

Project entry file: packages/protocol/src/main.ts

@tbdex/http-client

Project entry file: packages/http-client/src/main.ts

@tbdex/http-server

Project entry file: packages/http-server/src/main.ts

TBDocs Report Updated at 2024-03-27T01:40:09Z 7540d80

KendallWeihe · 2024-03-26T13:06:06Z

253 is implemented here in tbdex-js via the JSON Schema, yeah?

KendallWeihe

💯

nitro-neal · 2024-03-26T15:52:26Z

packages/protocol/src/dev-tools.ts

+          requiredPaymentDetails : {
+            $schema    : 'http://json-schema.org/draft-07/schema',
+            type       : 'object',
+            properties : {


do you normally need billing address for this?

I think in a real-life card payment the PFI might need the billing address, but for the purposes of this dummy test data it doesn't matter either way.

packages/protocol/src/dev-tools.ts

packages/protocol/src/message-kinds/rfq.ts

nitro-neal · 2024-03-26T16:01:31Z

packages/protocol/src/validator.ts

@@ -1,6 +1,6 @@
 import type { ErrorObject } from 'ajv'
 // validator functions are compiled at build time. check ./build/compile-validators.js for more details
-import * as compiledValidators from '../generated/compiled-validators.js'
+import * as compiledValidators from '../generated/compiled-validators.cjs'


only place in the code where we import a cjs right?

Maybe as a workaround / fix we can just make a normal .ts validator file? Why do we have to build a dynamic validation file?

@nitro-neal not all runtimes allow you to read files. this is especially the case in constrained CSP environments

Yep it's the only cjs import that I know of.

Maybe as a workaround / fix we can just make a normal .ts validator file?

I don't think we can, though there may be a neckbeard magic way. We're generating compiled-validators.*s using ajv's standaloneCode function. We're setting the esm: true option when generating the validation code, but it's still spitting out CJS for whatever reason. If we change the file extension to .ts, then that would technically be valid .ts since typescript is a superset of javascript. But the typescript compiler only knows how to transpile compiled-validators.ts into CJS. It can't turn that CJS-ts into ESM AFAIU.

Why do we have to build a dynamic validation file?

There's a few benefits to doing it this way, laid out in the ajv documentation here. But the gist is it's better for browsers to do this work at build time.

nitro-neal

Looks good! Just double check if that cjs is all good to go

Co-authored-by: nitro-neal <[email protected]>

kirahsapong · 2024-03-26T16:27:28Z

packages/protocol/tests/test-vectors.spec.ts

@@ -22,7 +22,7 @@ describe('TbdexTestVectorsProtocol', function () {
    expect(close.toJSON()).to.deep.eq(ParseClose.output)
  })

-  it('parse_offering', async() => {
+  it.skip('parse_offering', async() => {


Please don't skip tests.

There are no up-to-date test vectors for this test. We can't create functioning test vectors until 2 tbdex SDKs have implemented these changes. I would prefer to take the following approach:

merge these changes so this PR does not remain pending

implement or have another engineer implement these changes in JS and KT

Regenerate and validate the test vectors in question

Open a new PR against tbdex-js to add the vectors.

You should include this information in the PR description.

Fair enough. Added.

kirahsapong · 2024-03-26T16:27:49Z

packages/protocol/tests/test-vectors.spec.ts

@@ -70,7 +70,7 @@ describe('TbdexTestVectorsProtocol', function () {
    expect(quote.toJSON()).to.deep.eq(ParseQuote.output)
  })

-  it('parse_rfq', async () => {
+  it.skip('parse_rfq', async () => {


Ditto - Please don't skip tests.

Ditto #213 (comment)

kirahsapong · 2024-03-26T16:37:09Z

Re: using cjs -
Ajv injects require syntax in the generated code after introducing an externally resolved json schema $ref that appears twice in the offering schema. You either need to transform mixed syntax or use cjs or other. There are avenues

diehuxx · 2024-03-26T16:56:28Z

You either need to transform mixed syntax or use cjs or other. There are avenues

Which approach/avenue do you recommend? Does the approach I took fall into the category of using cjs or are you referring another possible approach?

kirahsapong · 2024-03-26T17:14:01Z

Which approach/avenue do you recommend? Does the approach I took fall into the category of using cjs or are you referring another possible approach?

Yes this approach falls in the category of using cjs.

diehuxx · 2024-03-26T17:35:39Z

Which approach/avenue do you recommend? Does the approach I took fall into the category of using cjs or are you referring another possible approach?

Yes this approach falls in the category of using cjs.

Which approach/avenue you recommend?

kirahsapong · 2024-03-26T17:49:31Z

Ajv injects require syntax in the generated code after introducing an externally resolved json schema $ref that appears twice in the offering schema. You either need to transform mixed syntax or use cjs or other. There are avenues

Which approach/avenue do you recommend? Does the approach I took fall into the category of using cjs or are you referring another possible approach?

Yes this approach falls in the category of using cjs.

Which approach/avenue you recommend?

You need to weigh the tradeoffs. I would think about what provides the smallest immediate lift with minimal drawbacks if getting this PR in shortly is a priority. If it isn't ideal longterm, I would consider that too.

mistermoe · 2024-03-26T19:34:56Z

@diehuxx we've configured ajv to output esm which y'all can see here. if ajv is returning some cjs that means there's a bug in their codegen. have we checked ajv github issues?

mistermoe · 2024-03-26T20:12:15Z

@diehuxx i would build dist and then set up a test script that consumes what's built to make sure that there's no issues. if all good on that front, i'd merge this and create an issue to tackle the ajv thing later.

diehuxx · 2024-03-27T00:44:47Z

@diehuxx we've configured ajv to output esm which y'all can see here. if ajv is returning some cjs that means there's a bug in their codegen. have we checked ajv github issues?

@mistermoe Good call. There's an open issue in ajv about esm generation being broken. ajv-validator/ajv#2209, but looks like a couple people posted workarounds in the comments. I'm going to give those a try.

diehuxx · 2024-03-27T01:40:46Z

Just pushed a brute force solution to the ESM compiled validators issue. I added some lines to packages/protocol/build/compile-validators.js which convert all requires into imports.

Diane Huxley added 5 commits March 25, 2024 13:44

Address tbdex #260

87c4162

Add name, group, and description to PaymentMethod

1d82302

Add min and max to PaymentMethod

85636b3

Add estimatedSettlementTime

80da7b6

Update tbdex to latest main and move compiled-validators to cjs

fceb7ac

diehuxx requested review from michaelneale, mistermoe, jiyoonie9, phoebe-lew, KendallWeihe and kirahsapong as code owners March 26, 2024 05:40

diehuxx commented Mar 26, 2024

View reviewed changes

audit fix

fcf4911

diehuxx changed the title ~~Issue 211~~ Recent small spec changes Mar 26, 2024

KendallWeihe approved these changes Mar 26, 2024

View reviewed changes

nitro-neal reviewed Mar 26, 2024

View reviewed changes

packages/protocol/src/dev-tools.ts Outdated Show resolved Hide resolved

nitro-neal reviewed Mar 26, 2024

View reviewed changes

packages/protocol/src/message-kinds/rfq.ts Outdated Show resolved Hide resolved

nitro-neal reviewed Mar 26, 2024

View reviewed changes

nitro-neal approved these changes Mar 26, 2024

View reviewed changes

Update rfq.ts

a70e13e

Co-authored-by: nitro-neal <[email protected]>

kirahsapong reviewed Mar 26, 2024

View reviewed changes

add seconds comment

e7e5408

Replace require with import in compiled validators

7540d80

diehuxx merged commit 02ce905 into main Mar 27, 2024
6 checks passed

diehuxx deleted the issue-211 branch March 27, 2024 02:03

diehuxx mentioned this pull request Mar 29, 2024

Add balance resource and todos #212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recent small spec changes #213

Recent small spec changes #213

diehuxx commented Mar 26, 2024 •

edited

Loading

changeset-bot bot commented Mar 26, 2024 •

edited

Loading

diehuxx Mar 26, 2024

nitro-neal Mar 26, 2024

codecov-commenter commented Mar 26, 2024 •

edited

Loading

github-actions bot commented Mar 26, 2024 •

edited

Loading

KendallWeihe commented Mar 26, 2024

KendallWeihe left a comment

nitro-neal Mar 26, 2024

diehuxx Mar 26, 2024

nitro-neal Mar 26, 2024

mistermoe Mar 26, 2024

diehuxx Mar 26, 2024

nitro-neal left a comment

kirahsapong Mar 26, 2024

diehuxx Mar 26, 2024 •

edited

Loading

kirahsapong Mar 26, 2024

diehuxx Mar 26, 2024

kirahsapong Mar 26, 2024

diehuxx Mar 26, 2024

kirahsapong commented Mar 26, 2024

diehuxx commented Mar 26, 2024 •

edited

Loading

kirahsapong commented Mar 26, 2024

diehuxx commented Mar 26, 2024

kirahsapong commented Mar 26, 2024

mistermoe commented Mar 26, 2024

mistermoe commented Mar 26, 2024

diehuxx commented Mar 27, 2024 •

edited

Loading

diehuxx commented Mar 27, 2024

Recent small spec changes #213

Recent small spec changes #213

Conversation

diehuxx commented Mar 26, 2024 • edited Loading

Commits

changeset-bot bot commented Mar 26, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Mar 26, 2024 • edited Loading

Codecov Report

github-actions bot commented Mar 26, 2024 • edited Loading

@tbdex/protocol

@tbdex/http-client

@tbdex/http-server

KendallWeihe commented Mar 26, 2024

KendallWeihe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitro-neal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diehuxx Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kirahsapong commented Mar 26, 2024

diehuxx commented Mar 26, 2024 • edited Loading

kirahsapong commented Mar 26, 2024

diehuxx commented Mar 26, 2024

kirahsapong commented Mar 26, 2024

mistermoe commented Mar 26, 2024

mistermoe commented Mar 26, 2024

diehuxx commented Mar 27, 2024 • edited Loading

diehuxx commented Mar 27, 2024

diehuxx commented Mar 26, 2024 •

edited

Loading

changeset-bot bot commented Mar 26, 2024 •

edited

Loading

codecov-commenter commented Mar 26, 2024 •

edited

Loading

github-actions bot commented Mar 26, 2024 •

edited

Loading

diehuxx Mar 26, 2024 •

edited

Loading

diehuxx commented Mar 26, 2024 •

edited

Loading

diehuxx commented Mar 27, 2024 •

edited

Loading