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 DID resolve to web5 cmd #77

Merged
merged 4 commits into from
Feb 27, 2024
Merged

Add DID resolve to web5 cmd #77

merged 4 commits into from
Feb 27, 2024

Conversation

KendallWeihe
Copy link
Contributor

Closes #70


Usage

web5 did resolve <uri>

For example, run web5 did:jwk create, and use the uri from the output as the input to the resolve command

Commentary

I think I may redo the DX for the did:jwk and did:web to be web5 did create <method> <additional args> ... @mistermoe thoughts? My driving reason is to try and match the namespacing in our various web SDKs (specifically looking at web5-js as the most mature among them)

I want to be able to resolve a DID with just the URI, so I would want the create commands to be consistent with this DX as well

cmd/web5/cmd_did.go Outdated Show resolved Hide resolved
return err
}

fmt.Println(string(jsonDID))
Copy link
Contributor

Choose a reason for hiding this comment

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

makes me wonder if this needs some sort of message or prettifying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah right now we're json.MarshalIndent()ing to make it pretty, but we could push this down to the SDK-level

Tangentially, I have a ticket for adding CLI args so the user can direct formatting #64

Copy link
Contributor

Choose a reason for hiding this comment

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

How were you thinking about exposing this at the sdk level @KendallWeihe ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, reiterating that the CLI is merely a wrapper around SDK functionality (it doesn't extend it)

#86

@@ -12,7 +11,7 @@ type didWebCreate struct {
Domain string `arg:"" help:"The domain name for the DID." required:""`
}

func (c *didWebCreate) Run(_ context.Context) error {
Copy link
Contributor Author

@KendallWeihe KendallWeihe Feb 26, 2024

Choose a reason for hiding this comment

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

Also found out (per PR feedback) we don't need these, Kong will dynamically inject based on needs

Copy link

@diehuxx diehuxx left a comment

Choose a reason for hiding this comment

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

Add to cmd/web5/README.md?

Also, I didn't realize we're building a CLI. Why are we now creating a web5 CLI even though we weren't considering that AFAIK when we built other implementations?

@diehuxx diehuxx closed this Feb 26, 2024
@diehuxx diehuxx reopened this Feb 26, 2024
@alecthomas
Copy link
Contributor

Add to cmd/web5/README.md?

Also, I didn't realize we're building a CLI. Why are we now creating a web5 CLI even though we weren't considering that AFAIK when we built other implementations?

I'm not sure of the intent, but Go is almost ideal for writing redistributable CLIs as it generates statically linked completely self-contained executables, so if we want a single CLI then this makes sense vs. being in another language. Rust would also be a good choice for the same reasons.

@mistermoe
Copy link
Contributor

mistermoe commented Feb 27, 2024

Add to cmd/web5/README.md?

Also, I didn't realize we're building a CLI. Why are we now creating a web5 CLI even though we weren't considering that AFAIK when we built other implementations?

@diehuxx it wasn't something that we necessarily deliberated. Happened a bit organically. 2 primary reasons.

  1. Did creation for PFIs will happen typically once. Having to write a throwaway script to do this feels unnecessary. Have run into this with 2 partners and for our own PFI. Figured a cli might make this easier
  2. I find myself needing to resolve DIDs for debugging purposes quite often using our own resolvers. Would be nice to use a command to do so

Further I imagine being able to use a cli for updating / managing a did in general will be helpful. Especially given that it'll be able to use aws KMS quite easily

@mistermoe
Copy link
Contributor

Re: question in description

Makes sense @KendallWeihe was actually thinking the same thing re: args

ideally we can surface method specific command line options & args based on the options we've surfaced at the sdk level

@alecthomas
Copy link
Contributor

A bit tangential, but I can highly recommend goreleaser for building and releasing Go binaries. It will automatically build and upload to GH releases. FTL uses it, for example.

@KendallWeihe
Copy link
Contributor Author

Add to cmd/web5/README.md?

Done, thanks! Though that README is kinda bleh, not sure what would be ideal there, but a problem for later.

@KendallWeihe
Copy link
Contributor Author

Something very strange is going on. Tests are failing on this branch; the TestVerifySECP256K1 test is failing with an error failed to parse public key: malformed public key: invalid length: 64. I tried re-running the workflow, but it failed again. Tests from the prior commit are passing, but all that was changed in the most recent commit was a README. Tests are passing locally. I branched off of this branch and reran the tests in the GitHub Pipeline and the tests passed.

I suspect we have some weird cryptographic issue going on where something to do with this particular commit, and the random generation made available in our GitHub Pipelines.

Going to merge this in and open a ticket.

@KendallWeihe KendallWeihe merged commit 6695ec4 into main Feb 27, 2024
2 of 3 checks passed
@KendallWeihe
Copy link
Contributor Author

surface method specific command line options & args based on the options we've surfaced at the sdk level

totally agree #85

@leordev leordev restored the kendallw/cmd-resolve-did branch February 28, 2024 21:18
@mistermoe mistermoe deleted the kendallw/cmd-resolve-did branch April 14, 2024 06:07
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.

Add resolve to web5 cmd
5 participants