Patchwork integration #54
Conversation
…lish merging this to `patchwork-integration` branch, as it is needed for forward-requests queries
Merging this to have a single branch for all things needed for patchwork integration
| return pull( | ||
| shardsFromOthers(), | ||
| // This query is specific to recovering SSB identities | ||
| pull.filter(shard => get(shard, 'value.content.attachment.name') === 'gossip.json'), |
There was a problem hiding this comment.
Wondering if we can't dynamically pass in filter functions, that way we don't have make this so restrictive...
There was a problem hiding this comment.
@KGibb8 so i just changed it that it takes a filter function as an argument.
probably it would be neater if this was a property of the opts object. but i wasn't sure if opts has a standardised form and this would meddle with it.
| shardsFromOthers(), | ||
| // This query is specific to recovering SSB identities | ||
| pull.filter(shard => get(shard, 'value.content.attachment.name') === 'gossip.json'), | ||
| pull.unique(s => get(s, 'value.author')), |
| const { isFeedId } = require('ssb-ref') | ||
|
|
||
| module.exports = function (server) { | ||
| return function bySecretOwner (secretOwner, opts = {}) { |
There was a problem hiding this comment.
this is forSecretOwner rather than bySecretOwner, because we're looking for all requests that have been created by person A, who may or may not be person B, and as person C, its a big semantic difference.
There was a problem hiding this comment.
im happy to change it. i was imaging its short for 'indexed by secret owner', like we've got a query for shards indexed 'by root'. but if you are seeing it as 'forward requests authored by secret owner' then thats not what we want.
|
I'm feeling a strong opinion cropping up re the use of the I've encountered a situation where a forward could possibly have two branch attributes, but there would be no way to determine the message type of those messages. Which makes for messy querying. e.g. the This far, we haven't implemented having the shard key / id in the branch section. However if we were to do it in future, we'd be looking at significant code changes (more I'd propose instead of using the {
type: 'dark-crystal/forward',
requestId: '%.........',
shardId: '%.........',
...
}So at this stage, rather than committing to insert the |
|
so i've ditched the branch field (we never used it anyway) and added shardId and requestId. one thing im not sure of is how to handle multiple forward requests (as forward requests dont contain specific information about what they are requesting). so as i've implemented it here, we will just take the first forward request the query gives as. but thinking about it probably the most recent one is more relevant. does it actually matter? i guess the only thing we are using this for is to determine if we have an outstanding forward request that we didn't already respond to. so maybe we should have a forward-request query for a given secret owner which pairs them with your responses and only tells you about unanswered ones. we could call it |
forward/async/publish.js
Outdated
| }) | ||
| pull( | ||
| pullForwardRequestsSecretOwner(get(shards[0], 'value.author')), | ||
| pull.collect((err, forwardRequests) => { |
There was a problem hiding this comment.
This change doesn't belong here, since its being used here in patchbay-darl-crystal - https://github.com/blockades/patchbay-dark-crystal/blob/master/views/forward/new.js#L99 - where we're not dealing with forward-requests. We either want to do this in a separate action i.e. forward.async.publishRequestReply (ew) or something like that. Or we make this forward publish function not care about getting data beforehand. I.e. it just simply formats the data (build) and publishes.
There was a problem hiding this comment.
@ameba23 unless this accounts for its use elsewhere?
There was a problem hiding this comment.
I've changed the Patchwork code to pass in only the rootId of the shard, so this should work for the moment. However, this is restricting as any forward responding to a request will always reference the first one that was returned from the query. See https://github.com/blockades/scuttle-dark-crystal/pull/54/files#r275383477
forward/async/publish.js
Outdated
| var requestId = null | ||
| if (err) return callback(err) | ||
| if (forwardRequests.length > 0) { | ||
| requestId = forwardRequests[0] |
There was a problem hiding this comment.
🔥
If Bob has got a forward-request from Alice2 and Claire, both claiming to be Alice, and Claire's arrived first, we send it to Alice2 (by passing in recp as an argument), but Claire's requestId gets attached instead of Alice2's. When Alice2 receives a forward and tries to cross-reference it with requests sent, the ID's don't match, and Alice2's client gets confused.
There was a problem hiding this comment.
We should instead be passing in the requestId, but that then buggers up the code in patchbay-dark-crystal. I'm much more in favour of a refactor, having an agnostic forward.async.publish method that just takes params (much like root.async.publish), and then either getting the front-end to do the work, or having separate publish actions for the two different contexts we're now dealing with (with requests - Patchwork / without requests - Patchbay)
forward/async/publish.js
Outdated
|
|
||
| module.exports = function (server) { | ||
| const pullShardsByRoot = PullShardsByRoot(server) | ||
| const pullForwardRequestsSecretOwner = PullForwardRequestsSecretOwner(server) | ||
| return function publishForward (root, recp, callback) { |
There was a problem hiding this comment.
It should be noted too that this breaks the pattern we use in the other publish functions, which expect a single params object, rather than two individual arguments.
the branch has the features needed to add SSB identity recovery to patchwork;
both these features should be useful for other things outside this specific use case, but for testing its useful right now to have them in one branch