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

Use generated wallet types for wallet functions #14

Closed
wants to merge 4 commits into from

Conversation

nathanmsmith
Copy link

@nathanmsmith nathanmsmith commented Jul 31, 2019

Replaces our (admittedly small) wallet types used in kbchat/wallet.go with generated types from our AVDL. I generated these files on the branch used in keybase/client#18623.

Note that chat types, while added in the types/ dir, remain unused by kbchat in favor of its own definitions. I'll change that in a future PR, but figured I'd try to minimize this diff as much as possible.

@nathanmsmith nathanmsmith requested a review from xgess August 2, 2019 17:45
@marceloneil
Copy link
Contributor

I guess this would remove the need for #17. Is there a reason for generating the types vs including them from keybase/client/go?

@nathanmsmith
Copy link
Author

@marceloneil We've been debating this internally, and I see the argument both ways. At least to start, I think it makes more sense to generate the type so people don't have to install an additional dependency/code they don't need and as a proof of concept for other languages, as we'd like to generate all of the same types for our Go, Python, and TypeScript bots. Our AVDL->Go compiler just happens to be the furthest along.

This very well could change in the future! Would love to hear your thoughts on it.

@marceloneil
Copy link
Contributor

Oh I see, it makes a lot of sense if you want to keep this implementation similar to other language implementations. How do you plan on dealing with versioning? Would you make releases to this package when the client makes a release?

@nathanmsmith
Copy link
Author

We're working on adding a task to the client repo to auto-update the bot repo types. We'll likely only push updates when additions or changes are made to the underlying API types, as there are a lot of types in the defined avro files that are used elsewhere by the app. When those happen, I'd actually expect the bot libraries to be updated shortly after changes hit in the nightly builds. I suppose we should talk about timing our stable bot releases with stable client releases, we haven't had that conversation yet!

@marceloneil
Copy link
Contributor

Sounds good, hope this gets merged in soon!

@nathanmsmith
Copy link
Author

speaking of which... bump @xgess? 🙂

@nathanmsmith
Copy link
Author

Closing this out in favor of #21

@joshblum joshblum deleted the nathan/PICNIC-104-generate-wallet-types branch February 24, 2020 15:40
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

2 participants