Add protocol version #191

kirahsapong · 2024-03-01T03:06:17Z

closes #172

changeset-bot · 2024-03-01T03:06:20Z

🦋 Changeset detected

Latest commit: f9cb8af

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@tbdex/protocol	Minor
@tbdex/http-client	Minor
@tbdex/http-server	Minor

Not sure what this means? Click here to learn what changesets are.

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

github-actions · 2024-03-01T03:07:47Z

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-12T19:09:09Z f9cb8af

kirahsapong · 2024-03-01T03:19:02Z

tests won't pass until TBD54566975/tbdex#244 is merged

diehuxx · 2024-03-03T04:14:30Z

packages/protocol/src/message.ts

@@ -44,6 +46,11 @@ export abstract class Message {
    return typeid(messageKind).toString()
  }

+  /** Gets the 'x.x' major/minor formatted version based on the current protocol release version */
+  static getProtocolVersion(): `${number}` {


Parsing this as a number is going to lead to weird comparisons, e.g. Version 1.2 > 1.12. What do you think of returning a string?

Oh wait this is the version of the SDK, not the TBDex version. I think those two versions will be different, no? What are we using the SDK version for within the SDK?

Parsing this as a number is going to lead to weird comparisons, e.g. Version 1.2 > 1.12. What do you think of returning a string?

this is a template literal type. it returns a number as a string. so we will get major and minor versions even if we pass '1.12.1' (we will get "1.12") and we will get a compiler error if we try to pass hello, diane! tl;dr: it is returning a string :)

Oh wait this is the version of the SDK, not the TBDex version. I think those two versions will be different, no? What are we using the SDK version for within the SDK?

yep realized afterward I conflated the two. there are weird gotchas with both so will be discussing more this week. its likely the source of the version will change but the rest in this pr will look about the same.

TIL template literals. thanks!

would this still allow number of any format to be passed in (i.e. 9000 or 0.12345?

the pattern we want is x.x like 1.1 or 2.0, per initial proposal https://hackmd.io/TucOUP8UQt-XKj8G9UsJ2Q ?

wow TIL template literal as well, very cool

packages/http-server/src/request-handlers/create-exchange.ts

packages/protocol/src/types.ts

codecov · 2024-03-11T23:35:51Z

Codecov Report

Merging #191 (f9cb8af) into main (1acffee) will increase coverage by 0.01%.
The diff coverage is 96.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #191      +/-   ##
==========================================
+ Coverage   93.22%   93.23%   +0.01%     
==========================================
  Files          37       37              
  Lines        3011     3047      +36     
  Branches      329      338       +9     
==========================================
+ Hits         2807     2841      +34     
- Misses        204      206       +2

Components	Coverage Δ
protocol	`94.27% <96.22%> (+<0.01%)`	⬆️
http-client	`94.83% <ø> (ø)`
http-server	`89.49% <ø> (ø)`

diehuxx · 2024-03-12T02:23:12Z

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

Suggestion: populate protocol within .create() rather than passing it as an parameter. This package only supports one possible version currently, so it could simplify DX.

agreed. i think protocol should be set internally and optionally overridable

So, default to 1.0 for now with no validation? Or hardcode the versions somewhere the way Kirah previously had it?

I'm not sure what you mean by no validation, but either option sounds sufficient to me

default to 1.0 with no validation feels like easiest path forward right now. wdyt @phoebe-lew ?

@mistermoe agree that's easiest.

@diehuxx if overridable with no validation, someone could put in 666.6 or not even a number as the version

we can create an issue to track validating protocol and tackle post 1.0. i think we'll have a better sense for what "validation" entails beyond just the actual value (e.g. is this message conformant to the version specified) when we're closer to introducing a change to the protocol

@mistermoe ah i see, thanks! agreed let's not do validation for now and we'll follow up on it later

issue created: TBD54566975/tbdex#270

.changeset/stale-hats-run.md

diehuxx · 2024-03-12T20:45:26Z

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

@@ -8,7 +8,7 @@ import { Parser } from '../parser.js'
 */
 export type CreateCloseOptions = {
  data: CloseData
-  metadata: Omit<CloseMetadata, 'id' | 'kind' | 'createdAt'>
+  metadata: Omit<CloseMetadata, 'id' | 'kind' | 'createdAt' | 'protocol'> & { protocol?: CloseMetadata['protocol'] }


Non-blocking: I kinda hate indexed access type syntax in typescript. Instead I prefer to just use the underlying type, in this case,

`${number}`

Suggested change

metadata: Omit<CloseMetadata, 'id' | 'kind' | 'createdAt' | 'protocol'> & { protocol?: CloseMetadata['protocol'] }

metadata: Omit<CloseMetadata, 'id' | 'kind' | 'createdAt' | 'protocol'> & { protocol?: `${number}` }

Up to you if you feel like doing this. Just a personal preference on my part.

Curious what you don't like about it! the way it looks, or is there something about the way it works that you dislike?

it's a readability thing. it's an abstraction that i personally don't find helpful. i don't like needing to have to look at a separate file or type to figure out what i'm actually working with, especially if it's as simple as a number or string.

Oh got it. I'm leaning towards leaving it as is because theoretically, the types could drift between CloseMetadata and this field if it's not referencing CloseMetadata.

diehuxx reviewed Mar 3, 2024

View reviewed changes

packages/http-server/src/request-handlers/create-exchange.ts Outdated Show resolved Hide resolved

diehuxx reviewed Mar 3, 2024

View reviewed changes

packages/protocol/src/types.ts Show resolved Hide resolved

kirahsapong and others added 10 commits March 7, 2024 11:51

add required field protocol version to message and resource

bd5ef13

change field to protocol

45afe32

add checks for protocol version to create or add to exchange

c99f83b

remove jwk from generate-test-vectors

0081e00

move protocol version check on create-exchange to create-rfq

046fa92

test rfq create

3112d1a

replace protocol version

0dee6b2

rename

7618aad

update test

93bffc6

pass in protocol version

21674a8

phoebe-lew force-pushed the feat/add-protocol-version branch from 9b4e633 to 21674a8 Compare March 7, 2024 20:57

phoebe-lew added 9 commits March 7, 2024 17:45

compile errors

2b09ff6

fix offering

202bea1

test fixes

e6f7154

update client

4fedae0

test fixes

e1295d9

revert client spec changes

0a3d4a2

http-client test

e63a804

add changesets

6981fce

update submodule version

1810dfd

phoebe-lew marked this pull request as ready for review March 11, 2024 23:35

phoebe-lew requested review from michaelneale, mistermoe and jiyoontbd as code owners March 11, 2024 23:35

phoebe-lew requested review from phoebe-lew and KendallWeihe as code owners March 11, 2024 23:35

diehuxx reviewed Mar 12, 2024

View reviewed changes

jiyoontbd reviewed Mar 12, 2024

View reviewed changes

.changeset/stale-hats-run.md Outdated Show resolved Hide resolved

jiyoontbd linked an issue Mar 12, 2024 that may be closed by this pull request

add version property to metadata for both Resource and Message TBD54566975/tbdex#159

Closed

phoebe-lew added 5 commits March 12, 2024 11:52

type changes

98a4bad

revert non-protocol changes

768dd44

remove redundant protocol

ea7a65c

remove redundant protocol

2bdd98c

nits

f9cb8af

diehuxx reviewed Mar 12, 2024

View reviewed changes

diehuxx approved these changes Mar 12, 2024

View reviewed changes

jiyoontbd approved these changes Mar 13, 2024

View reviewed changes

phoebe-lew merged commit 46481e7 into main Mar 13, 2024
14 checks passed

phoebe-lew deleted the feat/add-protocol-version branch March 13, 2024 14:56

This was referenced Mar 14, 2024

Version Packages #199

Merged

Version Packages #201

Closed

Version Packages #202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add protocol version #191

Add protocol version #191

kirahsapong commented Mar 1, 2024

changeset-bot bot commented Mar 1, 2024 •

edited

Loading

github-actions bot commented Mar 1, 2024 •

edited

Loading

kirahsapong commented Mar 1, 2024

diehuxx Mar 3, 2024

diehuxx Mar 3, 2024

kirahsapong Mar 3, 2024

diehuxx Mar 5, 2024

jiyoontbd Mar 12, 2024

KendallWeihe Mar 12, 2024

codecov bot commented Mar 11, 2024 •

edited

Loading

diehuxx Mar 12, 2024

mistermoe Mar 12, 2024

phoebe-lew Mar 12, 2024

diehuxx Mar 12, 2024

mistermoe Mar 12, 2024

phoebe-lew Mar 12, 2024

mistermoe Mar 12, 2024

diehuxx Mar 12, 2024

jiyoontbd Mar 13, 2024

diehuxx Mar 12, 2024

phoebe-lew Mar 12, 2024

diehuxx Mar 12, 2024

phoebe-lew Mar 12, 2024

	metadata: Omit<CloseMetadata, 'id' \| 'kind' \| 'createdAt' \| 'protocol'> & { protocol?: CloseMetadata['protocol'] }
	metadata: Omit<CloseMetadata, 'id' \| 'kind' \| 'createdAt' \| 'protocol'> & { protocol?: `${number}` }

Add protocol version #191

Add protocol version #191

Conversation

kirahsapong commented Mar 1, 2024

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

🦋 Changeset detected

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

@tbdex/protocol

@tbdex/http-client

@tbdex/http-server

kirahsapong commented Mar 1, 2024

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

codecov bot commented Mar 11, 2024 • edited Loading

Codecov Report

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

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

Choose a reason for hiding this comment

changeset-bot bot commented Mar 1, 2024 •

edited

Loading

github-actions bot commented Mar 1, 2024 •

edited

Loading

codecov bot commented Mar 11, 2024 •

edited

Loading