-
Notifications
You must be signed in to change notification settings - Fork 6
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
Remote Attachment Codec #105
Conversation
lib/src/client.dart
Outdated
@@ -72,6 +72,15 @@ class Client implements Codec<DecodedContent> { | |||
final AuthManager _auth; | |||
final ContactManager _contacts; | |||
final CodecRegistry _codecs; | |||
static final CodecRegistry _codecRegistry = CodecRegistry(); | |||
|
|||
static CodecRegistry get codecs => _codecRegistry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want this.
I'm assuming this is because you wanted access to the codecs from elsewhere but we don't keep a static registry.
Instead, there is already a _codecs
registry as part of the Client
instance.
(notes on how to use it within your codec below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main problem with that change is that if Client doesn't contain the proper codecs registered at the moment of trying to decode then it will fail, the only way to create a Client object is from a wallet or conversation, in Android and iOS is completely different, is possible to register codecs statically, I was trying to do the same as Android and iOS do. I will create a codec registry in that method, otherwise if I try to use the actual _codecs
variable in Client
is not possible to access from that point of the code and I will need to move it to a different level and again is going to be different from the other mobile implementations.
Please check
https://github.com/xmtp/xmtp-android/blob/main/library/src/main/java/org/xmtp/android/library/codecs/ContentCodec.kt
https://github.com/xmtp/xmtp-android/pull/45/files
in Everywhere there is a way to register a codec using Client.register
(static way) or getting the codecs of the Client
object.
if still my last changes are not good for you, please let me know and have a meeting to discuss the proper approach in order to make the change as you want :)
if (hasCompression()) { | ||
encodedContent = decompressContent(); | ||
} | ||
return Client.codecs.decode(encodedContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't belong here.
Instead it should live inside of CodecRegistry
(and possible encode
if we want to send w/ it too).
e.g. once you have an implementation of _decompress
you would use it inside CodecRegistry.decode
:
Future<DecodedContent> decode(xmtp.EncodedContent encoded) async {
var codec = _codecFor(encoded.type);
if (codec == null) {
if (encoded.hasFallback()) {
return DecodedContent(contentTypeText, encoded.fallback);
}
throw StateError(
"unable to decode unsupported type ${_key(encoded.type)}");
}
- return DecodedContent(encoded.type, await codec.decode(encoded));
+ var decompressed = _decompress(encoded);
+ return DecodedContent(decompressed.type, await codec.decode(decompressed));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High-level: flutter uses a per-instance codec registry not a global registry. I included some notes to get you started moving compression support into CodecRegistry
. I also left some notes on how to move RemoteAttachment.load
into Client.download(RemoteAttachment)
to simplify the dependency tangle.
I kept my notes limited to the incoming/read/download/decrypt flows. I'm happy to leave more notes on how you might approach the outgoing/write/upload/encrypt side (.sendMessage
etc) if that's helpful but I worried my wall-of-code comments might already be a bit much :)
if (hasCompression()) { | ||
encodedContent = decompressContent(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be the responsibility of CodecRegistry
to apply any supported compression schemes whenever the something needs to .decode
compressed content (and not just for remote attachments).
var codecRegistry = CodecRegistry(); | ||
codecRegistry.registerCodec(AttachmentCodec()); | ||
codecRegistry.registerCodec(RemoteAttachmentCodec()); | ||
return codecRegistry.decode(encodedContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this breaks the codec architecture in flutter (which uses per-instance codecs).
see note below re accepting a CodecRegistry
as part of RemoteAttachment.load
.
copy = EncodedContentCompression.DEFLATE.decompress(content) | ||
as EncodedContent; | ||
break; | ||
case Compression.COMPRESSION_GZIP: | ||
copy = EncodedContentCompression.GZIP.decompress(content) | ||
as EncodedContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this works (it looks like it's casting List<int>
as EncodedContent
)
extension EncodedContentCompressionExt on EncodedContentCompression { | ||
List<int> compress(List<int> content) { | ||
switch (this) { | ||
case EncodedContentCompression.DEFLATE: | ||
return zlib.encode(content); | ||
case EncodedContentCompression.GZIP: | ||
return gzip.encode(content); | ||
} | ||
} | ||
|
||
List<int> decompress(List<int> content) { | ||
switch (this) { | ||
case EncodedContentCompression.DEFLATE: | ||
return zlib.decode(content); | ||
case EncodedContentCompression.GZIP: | ||
return gzip.decode(content); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This responsibility probably belongs in CodecRegistry
-- which already orchestrates the other mechanics of encoding/decoding.
To put it into CodecRegistry
you could do something like this:
+ import 'dart:convert' as convert;
+ import 'dart:io' as io;
+ typedef Compressor = convert.Codec<List<int>, List<int>>;
class CodecRegistry implements Codec<DecodedContent> {
final Map<String, Codec> _codecs = {};
+ final Map<xmtp.Compression, Compressor> _compressors = {
+ xmtp.Compression.COMPRESSION_GZIP: io.gzip,
+ xmtp.Compression.COMPRESSION_DEFLATE: io.zlib,
+ }; // TODO: consider supporting custom compressors
...
Future<DecodedContent> decode(xmtp.EncodedContent encoded) async {
+ if (encoded.hasCompression()) {
+ var compressor = _compressors[encoded.compression];
+ if (compressor == null) {
+ throw StateError(
+ "unable to decode unsupported compression ${encoded.compression}");
+ }
+ var decompressed = compressor.decode(encoded.content);
+ encoded = xmtp.EncodedContent()
+ ..mergeFromMessage(encoded)
+ ..clearCompression()
+ ..content = decompressed;
+ }
var codec = _codecFor(encoded.type);
...
and then the registry can handle compression wherever it shows up.
static Future<EncryptedEncodedContent> encodedEncrypted( | ||
dynamic content, Codec<dynamic> codec) async { | ||
var secret = List<int>.generate( | ||
32, (index) => SecureRandom.forTesting().nextUint32()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.forTesting()
👀
dynamic load() async { | ||
var payload = await fetcher.fetch(url); | ||
if (payload.isEmpty) { | ||
throw StateError("No remote attachment payload"); | ||
} | ||
var encrypted = EncryptedEncodedContent( | ||
contentDigest, secret, salt, nonce, payload, contentLength, fileName); | ||
var decrypted = await decryptEncoded(encrypted); | ||
return decrypted.decoded; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably live on the Client
as a .download
method which can perform this download and already knows how to .decode
it:
class Client ...
...
Future<DecodedContent> download(RemoteAttachment attachment) async {
final Fetcher fetcher = HttpFetcher(); // TODO(perf): consider re-using
var payload = await fetcher.fetch(attachment.url);
if (payload.isEmpty) {
throw StateError("No remote attachment payload");
}
var decrypted = await attachment.decrypt(payload);
return decode(decrypted);
}
Note: I added .decrypt
to the RemoteAttachment
which is basically the same as your .decryptEncoded
but instead of static
it is an instance method and accepts the List<int> encryptedPayload
as a parameter instead. This lets you get rid of the intermediate class EncryptedEncodedContent
(which is basically just RemoteAttachment
+ List<int> payload
).
I'm going to pick this up and get it merged in a series of forthcoming PRs -- thank you for putting this together! (First PR is up w/ just the compression bits: #115) |
Expectations