DHT creation, refactor, and more #53
Conversation
feature/dht-create
|
yoooo @StonePack we appreciate you! 🙌🏾 |
|
Nice @StonePack !! will dig into this ASAP |
Awesome! I'm still learning the intricacies of web5 as I go and it's probably not perfect so any feedback would be amazing |
|
@StonePack regarding restructuring the package, specifically moving the reason i kept them outside is to avoid circular imports. the way you've got it makes it such that I'm relatively certain dart's compilation accommodates for circular imports but wasn't 100% sure so this may actually work totally fine. Though conceptually, was trying to keep the dependency tree acyclic to reduce cognitive overhead coming from other languages |
|
@mistermoe That makes sense, I tried to avoid circular imports but didn't even notice that one. I'll move those back out because you're right, even if the language handles it, it could still be confusing. |
|
@mistermoe Okay I moved |
| import 'package:keychain/keychain.dart'; | ||
| import 'package:web5/web5.dart'; | ||
|
|
||
| class IosKeyManager implements KeyManager, KeyImporter, KeyExporter { | ||
| @override | ||
| Future<String> generatePrivateKey(AlgorithmId algId) async { | ||
| final Jwk privateKeyJwk = await Crypto.generatePrivateKey(algId); | ||
| final String alias = privateKeyJwk.computeThumbprint(); | ||
|
|
||
| await Keychain.set(alias, json.encode(privateKeyJwk.toJson())); | ||
| return alias; | ||
| } |
There was a problem hiding this comment.
@mistermoe and @StonePack do we want web5-dart to be flutter aware? I feel like given that it's a "pure dart" package we probably should avoid dependencies that require Flutter.
Maybe an alternative here is to keep this IosKeyManager out of the dart package completely and when it's needed for a Flutter project we could add a web5-flutter package.
Maybe this repo can have the structure:
- packages
- web5-dart
- web5-flutter (this one would have the iOSKeyManager and Keychain stuff)
There was a problem hiding this comment.
I think that's a great call, this key manager could easily be added to a web5-flutter package which can be used in tandem with the web5-dart package. I'll pull it out for now and if tbd wants to host a web5-flutter package I can create a separate PR or I'll just stash it until that time comes.
There was a problem hiding this comment.
I've removed the iOS key manager code and created a new repo as reference for that work: https://github.com/stonepack/web5-flutter
|
Welcome to the TBD Community @StonePack - Thank you!! Great seeing you in the scoping call the other day. |
|
@ALRubinger I'm excited to be in the community, I look forward to attending more events! |
|
@mistermoe @wesbillman Just wanted to ping and see if there were any other changes or suggestions y'all had before I switch focus to working on other stuffs. |
|
@StonePack wrapping up review right now! |
|
@StonePack would it be possible to tease out the changes specific to DHT creation and open a smaller PR? would love to get that merged in ASAP bc it's awesome work. We can tackle restructuring the package, renaming files, and moving stuff around in subsequent PRs. |
You bet, I'll go ahead and close this PR and open another for DHT creation. |
I apologize for the large PR but I will create GitHub issues for now on and tackle smaller pieces via more focused PRs. As for this PR though I've made quite a few updates in hopes of keeping the dream alive.
Changelog:
createmethod (publishing still needs work, mainly the DNS packet creation)keychainand a class that extendsKeyManagerfor interacting with the Apple Keychainweb5-swiftrepoDnsHeaderclass