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

Make trezord-go usable as go library #147

Closed
wants to merge 27 commits into from
Closed

Make trezord-go usable as go library #147

wants to merge 27 commits into from

Conversation

karelbilek
Copy link
Contributor

@karelbilek karelbilek commented Feb 28, 2019

To help other devs to use trezord-go as a library, I have added an api/ folder, which can be now used as a native golang API/library to contact Trezor devices.

To "test" the API properly, I have also modified the server/ to use directly the API, and not the core/ calls.

api.New(..) now also initializes all the USB libraries, to make the use easier; that was originally in main.go

I have also cleaned up the main.go a bit by breaking it into more files, when I was already rewriting it significantly

There are some caveats.

  • I have intentionally made the API by copying the current HTTP calls and not making it more go-like; for example, Listen could be putting channel of devices instead of requiring previous, etc; basically because I want to "test" the new API calls by using it in the HTTP server itself.
  • There is still no documentation :)
  • There is no protobuf encoding/decoding, the API requires the user to do it themselves
  • There might be a problem with the USB initialization in api.New, since the user of the API might already have LibUSB initialized and libusb requires to be initialized just once.
  • There is no check if trezord-go is already running as a server. We should detect that and call that through HTTP if it already runs.
  • I have NOT tested it with other software that already used trezord-go in a weird way as a library. The only one that I know of is rfjakob/gocryptfs which used it via xaionaro-go/cryptoWallet. Someone should look at and rewrite xaionaro-go/cryptoWallet and rfjakob/gocryptfs and send them PRs before merging this.

The first commit is the addition of api/ and using it in server/ and trezord.go. The second commit is "just" moving everything except for api/ into internal/, so it cannot be linked directly by outside packages.

@karelbilek
Copy link
Contributor Author

Ugh. I run gofmt -s, which simplified the code but made it incompatible with older go versions. I... don't know if that's something important or not

@karelbilek
Copy link
Contributor Author

Also I broke fmt and metalinter. Hm. OK

@@ -7,7 +7,7 @@ import (
"net/http"
"regexp"

"github.com/trezor/trezord-go/core"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note - server now does not directly link to /core/, just through /api

@@ -121,7 +121,7 @@ func bcd2str(x uint16) string {

func indent(s string) string {
x := strings.Split(s, "\n")
for i, _ := range x {
for i := range x {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this broke the older go versions. :/

@karelbilek
Copy link
Contributor Author

github.com/conejoninja/tesoro/pb/messages has the protobuf messages, apparently

@karelbilek
Copy link
Contributor Author

... github.com/conejoninja/tesoro seems to be another API for trezor, that partly uses our trezord-go code by copy-pasting it. Maybe we should look at that too and try to somehow fit trezord-go-as-a-library there

@karelbilek
Copy link
Contributor Author

also CC @conejoninja , @xaionaro

@conejoninja
Copy link
Contributor

... github.com/conejoninja/tesoro seems to be another API for trezor, that partly uses our trezord-go code by copy-pasting it. Maybe we should look at that too and try to somehow fit trezord-go-as-a-library there

Hi, it's been a while since I last touched tesoro, apart from websub, what other part seems to be copy-pasted? I'll remove them.

@karelbilek
Copy link
Contributor Author

Hi, it's been a while since I last touched tesoro, apart from websub, what other part seems to be copy-pasted? I'll remove them

.Yeah I mean webusb :)

@karelbilek
Copy link
Contributor Author

Hm another self-review: if this is meant to be used as an API, api/ (and therefore, internal/core/ and all the others) should not get memorywriter (which is our internal thing) as an option, just either io.Writer or *log.Logger. Which will be some rewriting too.

@karelbilek
Copy link
Contributor Author

OK, now the library is actually trying to connect to bridge first and only then starts libusb. And from my short tests, it seems to be working.

Next steps: I will try to look at existing libraries/projects and try to use trezord-go/api there. Then add godoc documentation.

@karelbilek
Copy link
Contributor Author

Looking at the other libraries - some of them have the ability to work in Android. That's interesting. Trezord-go probably doesn't work in Android, but I am not so sure - it's Linux under the hood - so maybe with some tiny changes, this would work as a library under Android.

@karelbilek
Copy link
Contributor Author

I have tried to change Tesoro to use this new library, catched some bugs, but it seems working! (So it now works both with bridge, and if not installed, then with all 3 (~4) transports - hid, webusb, UDP emulator t2, UDP emulator for t1)

conejoninja/tesoro@master...karel-3d:master

It will be similar to change the other libraries. (Note - android is probably not working.)

To help other devs to use trezord-go as a library, I have added an api/ folder, which can be now used as a native golang API/library to contact Trezor devices.

To "test" the API properly, I have also modified the server/ to use directly the API, and not the core/ calls.

api.New(..) now also initializes all the USB libraries, to make the use easier; that was originally in main.go

I have also cleaned up the main.go a bit by breaking it into more files, when I was already rewriting it significantly

There are some caveats.

* I have intentionally made the API by copying the current HTTP calls and not making it more go-like; for example, Listen could be putting channel of devices instead of requiring previous, etc; basically because I want to "test" the new API calls by using it in the HTTP server itself.
* There is still no documentation :)
* There is no protobuf encoding/decoding, the API requires the user to do it themselves
* There might be a problem with the USB initialization in api.New, since the user of the API might already have LibUSB initialized and libusb requires to be initialized just once.
* There is no check if trezord-go is already running as a server. We should detect that and call that through HTTP if it already runs.
* I have NOT tested it with other software that already used trezord-go in a weird way as a library. The only one that I know of is rfjakob/gocryptfs which used it via xaionaro-go/cryptoWallet. Someone should look at and rewrite xaionaro-go/cryptoWallet and rfjakob/gocryptfs and send them PRs before merging this.
To prevent other packages to link directly into trezord internal packages as a library, as xaionaro-go/cryptoWallet is already doing (used inside rfjakob/gocryptfs)
This required splitting memorywriter into two objects; one implementing io.Writer and
responsible for rotating lines; one with the helper logging functions, which uses any io.Writer
So far by just adding nonsense comment. Not sure why Exclude doesn't work here. Maybe gometalinter bug?
This way, if someone uses trezord-go as a lib AND trezord is already running, the library
starts using bridge instead going lowlevel.

It is disabled in the bridge itself.
@karelbilek
Copy link
Contributor Author

I have added documentation (which creates nice-ish godoc files) and also added protobuf messages, so it's all less annoying to use.

I needed to write my own code generation tool, because otherwise, it's error-prone to "by hand" put together messages.MessageType_MessageType_ButtonAck and the proto type messages.ButtonAck (because go doesn't allow you to find that).

What COULD I do is to use some more dynamic protobuf library, but that would mean losing the type safety, so... yeah I have added a lot of auto-generated code. To make using of this as a library simplier.

From me it's all here, I will also modify Tesoro and xaionaro-go to use the final version, I will NOT look into go-ethereum, which reimplements the same usb things AGAIN, but it could use this library too. If someone has mood/time :))))

@karelbilek
Copy link
Contributor Author

karelbilek commented Mar 19, 2019

OK, this is tesoro using the new library - not the PB serialization, since that would take just too much time (the code would be cleaner, but I don't have infinite time)

https://github.com/karel-3d/tesoro/commits/master

This is @xaionaro cryptowallet using the new library, INCLUDING the PB serialization (it was mostly deletions, as most of the code is not necessary anymore)

https://github.com/karel-3d/cryptoWallet/commit/add0f509eaaf803effad0a24312a5ddf98c2d7c4

The example for using the new library, which shows all the major functions (this is really all that is needed to integrate Trezor to golang app, no need for custom USB handling and custom protobuf magic in every integration)

https://github.com/karel-3d/trezord-go-2/blob/1699ab6f898e22460f0b73b9c8f885727f592266/trezorapi/transport_test.go

@karelbilek
Copy link
Contributor Author

I am no longer updating this PR, and I would do it very differently this time (but still kept the auto-generation).

I will keep it here if someone wants to take over.

@trezor trezor deleted a comment from Dez6ztt Oct 2, 2021
@trezor trezor deleted a comment Oct 31, 2021
@karelbilek karelbilek closed this Sep 1, 2022
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