Conversation
…ernal notifications
# Conflicts: # src/fluree/db/api/transact.cljc # src/fluree/db/conn/memory.cljc # src/fluree/db/conn/remote.cljc # src/fluree/db/json_ld/commit_data.cljc # src/fluree/db/json_ld/reify.cljc # src/fluree/db/ledger/json_ld.cljc
zonotope
left a comment
There was a problem hiding this comment.
Looks good to me, although I'm still thinking through the ideas around
how nameservices relate to ledgers and chains of commits. In any case 👍🏾
| @@ -0,0 +1,98 @@ | |||
| (ns fluree.db.util.filesystem | |||
There was a problem hiding this comment.
This is similar to a lot of the work I was doing on my paused branch re-working our protocol surface and adding a storage protocol. I'm glad to see these changes to more isolate filesystem access, and I'm assuming we'll also use this ns for filesystem access in server too. I think there's still room to put all of this behind a general storage protocol to make memory access, ipfs access, s3 access, etc. more composable, but that can come later.
| :branch (if branch | ||
| (util/keyword->str branch) | ||
| "main") | ||
| :ns (mapv #(if (map? %) |
There was a problem hiding this comment.
tying commits to nameservices feels a little weird to me, but maybe because i'm just not used to the new concepts. In my mind, a nameservice maintains a mapping from ledger name to the head of a chain of commits, and that chain of commits should be valid whether or not someone reading them has access to a particular nameserver, or even if that nameserver has disappeared.
There was a problem hiding this comment.
Publishing a nameservice in a commit is just additional metadata, and actually should be optional. Here I've made it required, but I'd see us adding the ability to configure what (if any) nameservices get published in a commit.
I think most decentralized use cases should always point to a commit/db, never to a nameservice - as that is what a specific activity was based on at some moment in time.
If all I have is a commit however, it may be useful, or even critical, for some verification process to know if that data has been updated since. By having the nameservice(s) available in the commit, it is possible to see if the data publisher has subsequently updated information.
A good use case will be using a Fluree ledger to store your decentralized ID/DID. When using your DID, it is important for verification to know the public keys associated with it at the moment in time it was used to sign something. However, a verifier should also be able to consult the latest version to see if the public key was subsequently revoked because it was found to be compromised at the time it was used. Because the commit author supplied name service(s) in the commit, in this case the verifier can see what has changed since it was used and decide if those changes influence its opinion on its activity.
There was a problem hiding this comment.
I see. this makes sense. I think my mind is still stuck in the blockchain world where once something is published, you can't change it, but we can't rely on that here
There was a problem hiding this comment.
I think we have the best of both worlds. The commit is immutable and can stand on its own. Future updates are available if relevant/needed for the particular use case.
| @@ -0,0 +1,41 @@ | |||
| (ns fluree.db.method.remote.core | |||
There was a problem hiding this comment.
definitely not for this pr, but i think we should also eventually move all of the f.d.method.* related functionality behind a protocol.
There was a problem hiding this comment.
I don't love the method name here as well, but originally used it for consistency with the DID spec. I considered changing it in this PR, but tried to minimize changes ... and still not convinced it is the wrong name either. I'd welcome anyone changing the naming, structure, protocol, etc... probably needs a bit of all of those things.
| (-push [nameservice commit-data] "Pushes new commit to nameservice.") | ||
| (-subscribe [nameservice ledger-alias callback] "Creates a subscription to nameservice(s) for ledger events") | ||
| (-unsubscribe [nameservice ledger-alias] "Unsubscribes to nameservice(s) for ledger events") | ||
| (-sync? [nameservice] "Indicates if nameservice updates should be performed synchronously, before commit is finalized. Failure will cause commit to fail") |
There was a problem hiding this comment.
It feels like the information about which nameservice(s) should be synchronous should live on the conn instead of the nameservices themselves. The same nameservice type could be sync for one conn but async on another.
There was a problem hiding this comment.
It does live on the conn. A conn can get passed nameservices, else it will default to the nameservice we think makes the most sense for the conn. I did hard-code in the :sync? parameter on some, but ideally a developer can construct this however they like. So what you describe is how it operates.
zonotope
left a comment
There was a problem hiding this comment.
I wrote this comment yesterday in response to this comment but it didn't go through because I didn't realize I had started another review and needed to also click the "Review changes" button. In any case, it's not a big deal either way.
| (let [sync? (ns-proto/-sync? ns)] | ||
| (if sync? | ||
| (<? (ns-proto/-push ns commit-data)) | ||
| (ns-proto/-push ns commit-data)) |
There was a problem hiding this comment.
This is what i meant to refer to about the synchronous nameservers living on the conn instead of the protocol. Here were baking the behavior of the nameserver in relation to the connection in the nameserver protocol, and changing behavior outside the protocol based on the response of a protocol method.
I think a function called synchronous-nameservers or primary-nameservers that takes a conn and returns the list of nameservers that it needs to wait on, and a similar function that takes a conn and returns the list of nameservers that can be fire and forget makes more sense to me than calling a protocol method on each nameserver and doing a different thing based on what it returns.
That way the protocol only represents the generic behavior of a nameserver and the systems outside of the nameservers that rely on them don't leak their behavior into the nameserver itself. That will make them more modular.
There was a problem hiding this comment.
So if I understand correctly, are you saying that instead of instantiating a conn with a list of nameservers as an option (one of which should be synchronous), that we have two different option parameters on the conn, one called something like :synchronous-nameserver and one :asynchronous-nameservers, and then enforce the sync behavior based on what bucket the nameserver was placed into? If so, that's a fairly simple change.
There was a problem hiding this comment.
Right. and then we wouldn't need the sync? method on the nameserver protocol and the protocol would be more generic. Information about whether the nameserver is synchronous is in relation to the conn, not independent of it.
There was a problem hiding this comment.
Another thought, if a generic nameservice (and its corresponding ns type/method) can just live in the commit as metadata, then it perhaps doesn't need to be "hard coded" into the conn. The conn can have a set of nameservices it knows, but the individual ledger/commit is configured to write to specific ones - one of which is "primary" and always synchronous.
In this case, if you attempted to transact where the latest commit's nameservices are ones you know you can update, it will succeed - else it will throw an exception without you first changing the commit nameservices - not unlike how we allow defaultContext to get updated from time to time.
There was a problem hiding this comment.
Nameservices baked into the commit is still stretching my mental model and I'm still getting used to it, but I think I like this idea. It seems more flexible.
This extracts the name service from the connection, to ensure support of keeping query servers' local dbs up to date with remote
fluree/serverinstances. In addition to extracting the nameservice from the connection, it introduces:-notifymethod on the ledger, where a nameservice can notify a ledger that an updated commit exists to keep its in-memory db current, avoiding the need forfluree/load