Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add go package as a submodule #220

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Add go package as a submodule #220

wants to merge 21 commits into from

Conversation

w4ll3
Copy link
Member

@w4ll3 w4ll3 commented Sep 23, 2021

This adds a sub module to DIDKit didkit-go. The reason it is being added this way is to make it possible to be used as a module in a Go program. Once this is merged the instructions at didkit-go, which are simpler, will work.

Testing at DIDKit

  • Checkout to this branch
  • Init the submodule
git submodule update --init
  • Make sure to have the C target built
make -C extern/didkit/lib ../target/test/c.stamp
  • Run the tests
cd lib/didkit-go
go test

How to test it on a Go program

git init
  • Add DIDKit and SSI as submodules to your repo. (Since this is not merged yet you have to manually init the sub modules)
go get github.com/spruceid/didkit-go
git submodule add https://github.com/spruceid/didkit.git extern/didkit
git submodule add https://github.com/spruceid/ssi.git extern/ssi
cd extern/didkit
git checkout feature/go-package
git submodule update --init
cd ../ssi
git submodule update --init
cd ../..
  • Build DIDKit C target
make -C extern/didkit/lib ../target/test/c.stamp
  • Update the lib target
go mod edit -replace=github.com/spruceid/didkit-go=./extern/didkit/lib/didkit-go
  • Write some code to test the lib
package main

import (
        "fmt"

        "github.com/spruceid/didkit-go"
)

func main() {
        fmt.Println(didkit.GenerateEd25519Key())
}

Testing example application

  • Checkout to this branch
  • Init the submodule
git submodule update --init
  • Follow the instructions in the example README

@w4ll3 w4ll3 requested review from clehner and wyc September 23, 2021 19:07
Copy link
Contributor

@clehner clehner left a comment

Choose a reason for hiding this comment

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

Nice! Go tests passed. Example program builds and runs. For the example program, instead of using submodules for ssi and didkit, I used symlinks to my existing copies of them.

Other ideas:

  • If users of the didkit go package must have a local copy of ssi and/or didkit (as submodules or otherwise), could it be useful to have an example repo demonstrating this usage?
  • For the other didkit FFI libraries we usually have tests in GitHub actions. If not adding them here, should an issue be opened for this?
  • The other didkit interfaces are in this repo directly, while this one uses a separate repo which is included as a submodule. Could it have been included directly? I assume it did not work to do so, but wonder why.

@w4ll3 w4ll3 marked this pull request as draft September 24, 2021 13:10
@w4ll3
Copy link
Member Author

w4ll3 commented Sep 24, 2021

  • If users of the didkit go package must have a local copy of ssi and/or didkit (as submodules or otherwise), could it be useful to have an example repo demonstrating this usage?
  • You mean like a Hello World project for didkit-go?
  • The other didkit interfaces are in this repo directly, while this one uses a separate repo which is included as a submodule. Could it have been included directly? I assume it did not work to do so, but wonder why.
  • It seems that go packages must be at the root of the repo, that's why it's added as a sub module

@w4ll3 w4ll3 marked this pull request as ready for review September 24, 2021 13:45
@clehner
Copy link
Contributor

clehner commented Jan 6, 2022

CI was added; thanks @w4ll3. Tests still pass after merging in changes from main (2eb854d). The submodule didkit-go repo looks good to me.

I tried using the example more, and was able to get a verifiable credential, but was not able to present it. Also, I don't know if it makes sense to use a DID as the verifiable credential id. Usually a DID is an id of a DID document, and a verifiable credential is not a DID document. In this case the keypair associated with the DID used in the VC id is not stored, so it does not appear to serve a useful purpose (e.g. for signing/verification). A UUID could be used instead - or the id property could be removed if the app doesn't need to store/retrieve the credentials by id. For the sake of enabling didkit-go, I think this PR could be merged as-is and new issues created to track these things.

@CLAassistant
Copy link

CLAassistant commented Oct 27, 2022

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants