Federation Data Exchange for FE2 #1943
Conversation
Pull reviewers statsStats of the last 30 days for popstellar:
|
| "failure" -> { | ||
| require(reason != null) { "Reason must be provided for failure status." } | ||
| require(publicKey == null) { "Public key must be null for failure status." } | ||
| } | ||
| "success" -> { |
There was a problem hiding this comment.
Perhaps it would be better to define these ("failure" and "success") as static constants, so in case we want to change them in future it's just 1 place to modify
There was a problem hiding this comment.
Ok, I'll change that
| addLinkedLao(laoId, otherLaoId, tokens) | ||
| newTokensNotifyFunction?.invoke(laoId, otherLaoId, rollCallId, tokens) |
There was a problem hiding this comment.
If you take a look at other repos (any one, e.g. rollcallrepo) you'd see that in general this is not the preferred way to notify modifications on a given data structure, but that we use java rx constructs (subject, observables and so on)
There was a problem hiding this comment.
Well the thing is that I tried using observables and for some reason I had many problems with the UI not updating or weird other issues... That's why in the end I changed to using callbacks, I know it's a bit more complex but at least it's working
There was a problem hiding this comment.
Sure, we can keep it as is then
| // Displaying the linked organizations | ||
| val laos = linkedOrganizationsViewModel.getLinkedLaosMap().keys | ||
| displayLinkedOrganizations(laos) | ||
| linkedOrganizationsViewModel.doWhenLinkedLaosIsUpdated { laoId, laoMap -> | ||
| if (laoId == laoViewModel.laoId) { | ||
| CoroutineScope(Dispatchers.Main).launch { | ||
| val currentLaos = laoMap.keys | ||
| displayLinkedOrganizations(currentLaos) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| linkedOrganizationsViewModel.setLinkedLaosNotifyFunction() |
There was a problem hiding this comment.
Why calling display first with the current list and then putting a custom and complex observer that uses coroutines and not directly using a simple observer? This relates to my previous comment on how to deal with modifications using observables and subjects
| get() { | ||
| val laoIds = listOf(laoId) + linkedOrgRepo.getLinkedLaos(laoId).keys | ||
| val laoObservables: MutableList<Observable<MutableList<Chirp>>> = ArrayList() | ||
| for (lao in laoIds) { | ||
| val observable = | ||
| socialMediaRepository | ||
| .getChirpsOfLao(lao) | ||
| .map { ids: Set<MessageID> -> | ||
| val chirps: MutableList<Observable<Chirp>> = ArrayList(ids.size) | ||
| for (id in ids) { | ||
| chirps.add(socialMediaRepository.getChirp(lao, id)) | ||
| } | ||
| chirps | ||
| } | ||
| .flatMap { observables: List<Observable<Chirp>> -> | ||
| Observable.combineLatest(observables) { chirps: Array<Any?> -> | ||
| Arrays.stream(chirps) | ||
| .map { obj: Any? -> Chirp::class.java.cast(obj) } | ||
| .sorted( | ||
| Comparator.comparing { chirp: Chirp? -> | ||
| if (chirp != null) -chirp.timestamp else 0 | ||
| }) | ||
| .collect(Collectors.toList()) | ||
| } | ||
| } | ||
| laoObservables.add(observable) | ||
| } | ||
| val combinedObservables = | ||
| Observable.combineLatest(laoObservables) { chirpsLists: Array<Any?> -> | ||
| Arrays.stream(chirpsLists) | ||
| .flatMap { chirpsList: Any? -> (chirpsList as List<Chirp>).stream() } | ||
| .sorted( | ||
| Comparator.comparing { chirp: Chirp? -> | ||
| if (chirp != null) -chirp.timestamp else 0 | ||
| }) | ||
| .collect(Collectors.toList()) | ||
| } | ||
| // We want to observe these changes on the main thread such that any modification done | ||
| // to the view are done on the thread. Otherwise, the app might crash | ||
| return combinedObservables.observeOn(schedulerProvider.mainThread()) | ||
| } |
There was a problem hiding this comment.
This getter will be executed every time we access that field, which looks very heavy as getter method. You could precompute this variable in initialization, such that this function is only called once
There was a problem hiding this comment.
Isn't this function supposed to be called every time ? I'm not that familiar with the social media implementation (I just modified a bit this function for chirps to be compatible with federation), but from what I understood it has to be executed every time to get new chirps, adapt the chirp list if there is a new roll call and now, with the newly added code, adapt the chirp list in case there is a new linked LAO
I'll look a bit more into that...
There was a problem hiding this comment.
Yeah sorry I was missing the context of execution of that function, now I looked it up better and it's how it's supposed to work, even if it's not very efficient and perhaps in the future we could change it
| linkedOrgRepo.addLinkedLao(laoId, linkedOrgRepo.otherLaoId!!, arrayOf()) | ||
| laoRepo.addDisposable( | ||
| context.messageSender |
There was a problem hiding this comment.
Maybe for better separability it would be nice to have an ad-hoc disposable in the linked org repo, without using the lao one
There was a problem hiding this comment.
Yes, I'll do that
There was a problem hiding this comment.
Could you add a test when also there are some organizations (so not just the empty case)?
There was a problem hiding this comment.
Matteo's comments cover all the important parts but I have one last request:
FederationResult and TokensExchange are new message data, that require validation of inputs.
You should add the validation using MessageValidation.validate() to avoid having technical debt, as total validation is a goal documented in #1751. You should also update this Issue and add the new data in the expandable columns :)
You can check in #1952 how to use validation, and how to create simple tests to check the constructor.
| tokenExchange.tokens.forEach { t -> | ||
| laoRepo.addDisposable( | ||
| disposables.add( |
There was a problem hiding this comment.
Using the disposable from the handler doesn't give you the opportunity to clear the added disposables when closing a lao or the application. Take a look at the usage of disposables in the RollCallRepository or any other repository.
There was a problem hiding this comment.
Oh ok, then should I create the disposables in LinkedOrganizationsRepository (as done in RollCallRepository) and then call it from the LinkedOrganizationsHandler ? Or should I do it directly in the handler ?
There was a problem hiding this comment.
Yep you can do it from the repository as for roll calls and keep it consistent
| ) : Data { | ||
|
|
||
| init { | ||
| when (status) { |
There was a problem hiding this comment.
This looks like input validation, maybe you should complete MessageValidator class to check a federation status? Validation would then just be a clear oneliner 👍
You also probably want to check the public key to be non empty B64
| ) : Data { | ||
|
|
||
| init { | ||
| MessageValidator.verify().isBase64(rollCallId, "rollCallId") |
There was a problem hiding this comment.
isBase64 (as every other functions in MessageValidator) returns the builder so you can write :
MessageValidator.verify().isBase64(rollCallId, "rollCallId").validPastTimes(timestamp)
There was a problem hiding this comment.
given the documentation of the protocol :
consensus
// ../protocol/query/method/message/data/dataFederationTokensExchange.json
{
"$schema": "http://json-schema.org/draft-07/schema#",
"$id": "https://raw.githubusercontent.com/dedis/popstellar/master/protocol/query/method/message/data/dataFederationTokensExchange.json",
"description": "Sent by an organizer client to its server, to broadcast the Pop tokens",
"type": "object",
"properties": {
"object": {
"const": "federation"
},
"action": {
"const": "tokens_exchange"
},
"lao_id": {
"type": "string",
"contentEncoding": "base64",
"$comment": "Hash : HashLen(organizer, creation, name)"
},
"roll_call_id": {
"type": "string",
"contentEncoding": "base64",
"$comment": "last roll call id"
},
"tokens": {
"description": "[Array[Base64String]] list of Pop tokens",
"type": "array",
"uniqueItems": true,
"items": {
"type": "string",
"contentEncoding": "base64"
},
"$comment": "List must be sorted according to byte encoding: -,0...9,A...Z,_,a...z"
},
"timestamp": {
"type": "integer",
"description": "[Timestamp] of the tokens' exchange",
"minimum": 0
}
},
"additionalProperties": false,
"required": [
"object",
"action",
"lao_id",
"roll_call_id",
"tokens",
"timestamp"
]
}
- It looks like that laoId should be checked for
notEmptybase64. - I am not sure if an empty rollCallId makes sense, if not then switch to
notEmptybase64as well. - Tokens look like they individually need to be checked as well (they should be base64, maybe we wan't them not empty as well?). Also they should be checked for uniqueness. Is a null or empty array acceptable? This maybe needs to be defined.
| val exception = assertThrows(IllegalArgumentException::class.java) { | ||
| FederationResult(SUCCESS, publicKey = PK.encoded, reason = "reason", challenge = MG_CHALLENGE) | ||
| } | ||
| assert(exception.message == "Reason must be null for success status.") |
There was a problem hiding this comment.
maybe we want to add those strings in Constants to avoid failing if the message comes to change
| import org.junit.Test | ||
| import java.time.Instant | ||
|
|
||
| class TokensExchangeTest { |
There was a problem hiding this comment.
don't forget to add constructors succeed and failing tests as in the other class
|
|
|
|
quadcopterman
dismissed
Kaz-ookid’s stale review
Added complete message validation
Here is the implementation of the data exchange protocol for FE2 Android. The data exchange protocol is defined in issue #1802