Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

Publish prep #64

Merged
merged 10 commits into from
Oct 24, 2023
Merged

Publish prep #64

merged 10 commits into from
Oct 24, 2023

Conversation

mistermoe
Copy link
Member

@mistermoe mistermoe commented Oct 23, 2023

What's Changed

  • explicitly set minimum compatible java version to java 17
  • add jitpack config file to configure runner to use java 17
  • remove redundant version declarations in packages
  • fix message and resource digest computations
    • sha-256 hashing the canonicalized json was missing
  • verify - default assertionMethods to an empty list if null

@mistermoe
Copy link
Member Author

@jiyoontbd thoughts on making e2e file an integration test of http-client? we can do so by adding an integration-tests package to http-client/src. Otherwise we'll need to shuffle some stuff around in the build.gradle.kts files so that the e2e package isn't published.

@mistermoe
Copy link
Member Author

@michaelneale is the current state of httpserver taking ktor.server for a test drive to see what it's capable of? if so, thoughts on moving your findings to a branch or issue? thinking about removing the module until there's some meat on the bones

@mistermoe mistermoe marked this pull request as ready for review October 24, 2023 08:44
@michaelneale
Copy link
Contributor

Yep the server is just a placeholder - can move it

@michaelneale
Copy link
Contributor

michaelneale commented Oct 24, 2023

@mistermoe yeah after you merge (or part of this) could move it out, or just don't publish the server? #23 is the issue BTW. I opened up branch: https://github.com/TBD54566975/tbdex-kt/tree/impl-server-scaffold - which has the changes as they were, if you remove them from this branch, just open that issue with it linked to it for future reference if you like.

@@ -6,7 +6,6 @@ import io.ktor.server.application.Application
import io.ktor.server.testing.handleRequest
import io.ktor.server.testing.withTestApplication
import org.junit.jupiter.api.Test
import tbdex.server.tbdex.sdk.http_server.module
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add UnusedImports to our detekt config? @phoebe-lew any thoughts?

style:
  UnusedImports:
    active: false

@jiyoonie9
Copy link
Contributor

@jiyoontbd thoughts on making e2e file an integration test of http-client? we can do so by adding an integration-tests package to http-client/src. Otherwise we'll need to shuffle some stuff around in the build.gradle.kts files so that the e2e package isn't published.

Totally good with me if you'd like to move the e2e.kt into httpclient in this PR :)

@@ -51,7 +51,7 @@ object CryptoUtils {
// or just `#fragment`. See: https://www.w3.org/TR/did-core/#relative-did-urls.
// Using a set for fast string comparison. DIDs can be long.
val verificationMethodIds = setOf(parsedDidUrl.didUrlString, "#${parsedDidUrl.fragment}")
val assertionMethods = didResolutionResult.didDocument.assertionMethodVerificationMethodsDereferenced
val assertionMethods = didResolutionResult.didDocument.assertionMethodVerificationMethodsDereferenced ?: listOf()
Copy link
Contributor

Choose a reason for hiding this comment

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

could also do emptyList() :)

Copy link
Member Author

Choose a reason for hiding this comment

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

oo good call!

Copy link
Member Author

Choose a reason for hiding this comment

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

done 802f4ce thanks @jiyoontbd !

@KendallWeihe
Copy link
Contributor

Do we want to go ahead and bump to

implementation("com.github.TBD54566975:web5-kt:0.0.5-beta")

@mistermoe
Copy link
Member Author

yep good call @KendallWeihe. on it

@mistermoe
Copy link
Member Author

@KendallWeihe bumped. thoughts on surfacing web5-kt via api dependency instead of implementation? prevents people from having to include both as deps

@KendallWeihe
Copy link
Contributor

@KendallWeihe bumped. thoughts on surfacing web5-kt via api dependency instead of implementation? prevents people from having to include both as deps

Yeah it's the same question we're puzzling over in tbdex-js with regards to the peer dependencies. I think it's sort of a six of one, half a dozen to the other. I do think I side with the peer dependency approach because of one particular reason, I outlined it in this comment.

I thought more about this and am fully bought in with this approach. The primary reason being Developer experience. Sure it's a slightly less good experience to have to install two packages. But, it makes the distinction between tbdex the transport agnostic protocol and the specific implementation over http much less ambiguous. Which is to say we're making it clear that tbdex isn't married to http.

@mistermoe mistermoe merged commit a2e2116 into main Oct 24, 2023
1 check passed
@mistermoe mistermoe deleted the publish-prep branch October 24, 2023 16:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants