-
Notifications
You must be signed in to change notification settings - Fork 51
Conversation
Good thinking, @NunoAlexandre. So I would sum up the majority of this PR as being mechanical changes. Mostly revolving around the new data types Some interesting changes that needed to be made were involving the working copy creation (checkout and create). The interactions with git -- known as local transport because they are local interactions between the monorepo and a working copy -- are all done through The removal of cypress tests is due to some interactions being idempotent (taking the same action twice has no effect). So creating the same project or person twice does nothing, and the same goes for checking out a project. One notable change in the subroutine code was that @xla added an error variant for the default owner having to be set. This is because a lot of interactions need a default owner. Aside, we'd like to explore a more correct-by-construction approach to that post PR. That's all I can think of for now, but if anything comes up I'll add it in a follow-up comment. |
Thank you, Fintan! Will give it a review tomorrow morning if that works for you. |
Looking real good! Gave it a spin already to familiarise myself with the new backend and noticed a couple of things:
|
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.
Looks solid overall!
Some questions/thoughts:
The removal of cypress tests is due to some interactions being idempotent (taking the same action twice has no effect). So creating the same project or person twice does nothing, and the same goes for checking out a project.
-
This is great to have correct by design! However, I'm wondering if it wouldn't be better to have the tests in place to validate such property?
-
What is the rationale behind the name
LocalIdentity
? I see that wraps aVerifiedPerson
, which seems rather easier and less obscure to understand. Understanding it might help me build a better mental model.
proxy/api/src/http/error.rs
Outdated
), | ||
create::validation::Error::Remote(_) => ( | ||
StatusCode::INTERNAL_SERVER_ERROR, | ||
"NO_REMOTE", |
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.
Some of these string representations of the errors are suffixed with _ERROR
, others not. Is that intentional?
A |
That makes sense reading, but I am specifically talking about the type definition: pub struct LocalIdentity(VerifiedPerson);
// where
pub type VerifiedPerson = VerifiedIdentity<PersonDoc>;
Having the type definitions in mind, I am not sure how you mean this or how the two come together, as I'd assume from that sentence that |
@NunoAlexandre
Why is that? A union of what? Look: From the local perspective, there is one and only one keypair to use with Likewise, there is nothing which prevents you (or anyone else) from creating multiple identity documents and adding that key to it, for example on another device -- to prevent someone else from impersonating as you, there is the additional constraint that the It is not that you gain anything by doing this -- after all, anyone can correlate them by the key, so it doesn't make sense to e.g. create "nuno" and "nuno-work" with the expectation that this would be privacy-preserving. However, if we were to enforce on the protocol level that any given public key can appear in one and only one identity, every peer would have to keep track of the mapping key -> identity, and if a duplicate is detected... do what? Stop replicating anything signed by that key? You see that this would create additional complexity with dubious benefits -- for example, how do you bubble up to the user that such a conflict was detected, and therefore some data which they'd expect to be there is not there? How do you recover from such a situation? Since it is not harmful per-se to have multiple personalities (it may only confuse the people you interact with), we thus allow key -> identity to have a |
We are discussing this at different levels. My confusion comes from a place were, given that "Identity", in Link, can take the form of a Person, a Project, etc, calling Using an analogy to ease this out, say that we'd have the notion of Edit: This is out of scope for this PR, sorry for the noise, happy to have it removed. |
Your confusion seems to be that
|
Oh I get it: what throws you off is "identity". If the type was called |
Exactly :) it's not a big deal, I just wanted to understand if there was more to it that I should learn about. And there was :) thanks for bearing! |
proxy/coco/tests/working_copy.rs
Outdated
} | ||
|
||
#[tokio::test] | ||
async fn can_checkout() -> Result<(), Box<dyn std::error::Error>> { |
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.
@NunoAlexandre you mentioned validating the properties, here is the relevant one for radicle-upstream
. For project and person creation, these are properties in librad
and I believe are tested there.
36d552d
to
7694e12
Compare
Good shout 👍
So the default
We weren't going far enough when checking that a peer is a contributor. We were only checking that All fixes for those issues have been pushed now :) |
proxy/coco/src/peer/subroutines.rs
Outdated
.await | ||
.ok(); | ||
|
||
match state.clone_project(url.clone(), None).await { | ||
match state | ||
.clone_project(urn.clone(), remote_peer, None, None) |
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.
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.
Could you expand on what potential scenarios look like that would constitute runtime configuration changes? The reason I'm asking is that neither this comment or the original commit message reason about the why such a capability is desried.
Any thoughts on how this could be injected into the subroutines at runtime? Another channel perhaps?
Yeah usually configuration changes over time would come through some form of channel. For the configured seeds we use a watch
channel to provide last value semantics. Would need to look deeper into this use-case to give proper recommendation. There might be a general need for a latest configuration made to be known to the internals of the peer.
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.
So a use case would be the following scenario.
We ask to clone the Linux project, but as we're doing so the fetcher throws an error and says we've exceeded the configured limits. We're sure that this really is the Linux project and not a bunch of spammy data, so we configure our limits to go higher, try the replication again, and once we've got the project we can reset the limit.
Does that help shed some light on the question here?
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.
Somewhat. Will the limit go down again after the linux kernel has been replicated from pc-torvalds01
. By the sound of it this has to do with trust and spam protection. Which seems odd to be bound to runtime configuration changes. As in where would the signal come from that the fetch limit can be increased. I don't want to challenge the scenario too specifically, just curious to see how the concerns align.
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.
Somewhat. Will the limit go down again after the linux kernel has been replicated from pc-torvalds01.
Ya you would want to turn it down again.
By the sound of it this has to do with trust and spam protection.
Exactly, it's exactly a trust mechanism.
Which seems odd to be bound to runtime configuration changes. As in where would the signal come from that the fetch limit can be increased.
As @kim and I deliberated on it, we landed on "sane" default values and should be configurable depending on the scenario -- giving the malkovich the final say when using replicate
.
How are you thinking about it?
@@ -239,13 +259,15 @@ impl Repository { | |||
let path = path.join(name); | |||
log::debug!("Setting up new repository @ '{}'", path.display()); | |||
let repo = Self::initialise(path, description, &default_branch)?; | |||
log::debug!("REPO PATH: {}", repo.path().display()); |
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.
👀
Note to self:
|
Fixed in bcad1d9 |
Seems like the seed isn't replicating |
coco::blob(&mut browser, Some(revision.clone()), path, None) | ||
}) | ||
.await?; | ||
let arrows = "text/arrows.txt"; |
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 renaming here was clippy giving out about name shadowing of path
.
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.
@geigerzaehler @MeBrei @rudolfs I've left my inline summary of portions of the code that are probably worth more attention. Thank you and good luck 🙇
/// Peer [`librad::net::peer::RunLoop`] to advance the network protocol. | ||
run_loop: RunLoop, | ||
/// Underlying state that is passed to subroutines. | ||
state: State, |
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.
net::peer::Peer
is the new State
. It's essentially what was the PeerApi
, but a simplified and less footgun-y version
@@ -74,21 +84,27 @@ impl MaybeFrom<&Input> for Event { | |||
Input::Announce(input::Announce::Succeeded(updates)) => { | |||
Some(Self::Announced(updates.clone())) | |||
}, | |||
Input::Peer(event) => match event { | |||
PeerEvent::GossipFetch(FetchInfo { |
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.
There's no PeerEvent
anymore so we moved this logic to here https://github.com/radicle-dev/radicle-upstream/pull/1463/files#diff-463e58bd670bb12315d14a2f26fbfc88e97a22551753a25ecd20fe3056fe9de3R89
proxy/coco/src/peer/run_state.rs
Outdated
synced: usize, | ||
/// Number of synchronisation underway. | ||
syncs: usize, | ||
/// The set of failed syncs. |
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.
We now keep track of the PeerId
s we've interacted with for the syncing process on startup
@@ -140,9 +158,10 @@ pub struct RunState { | |||
/// `Connected(Peer1) -> Connected(Peer1) -> Disconnecting(Peer1)` | |||
// | |||
// FIXME(xla): Use a `Option<NonEmpty>` here to express the invariance. | |||
connected_peers: HashMap<PeerId, usize>, |
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.
We no longer get messages about Connected
and Disconnected
so we don't need this HashMap
. Instead, we can poll for the set of connected peers
/// Current internal status. | ||
pub status: Status, | ||
stats: net::protocol::event::downstream::Stats, |
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.
Stats
is what we can poll for Peer
information, such as the connected peers
proxy/coco/src/peer/run_state.rs
Outdated
/// Handle [`input::Sync`]s. | ||
fn handle_peer_sync(&mut self, input: &input::Sync) -> Vec<Command> { | ||
if let Status::Syncing { synced, syncs } = self.status { | ||
if let Status::Syncing { |
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 logic is changing here because of the move from usize
to HashSet<PeerId>
match (&self.status, event) { | ||
// Go from [`Status::Stopped`] to [`Status::Started`] once we are listening. | ||
(Status::Stopped { .. }, ProtocolEvent::Listening(_addr)) => { |
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.
Listening
doesn't exist anymore and it's now Endpoint::Up
, with Endpoint::Down
being its inverse
self.status = Status::Started; | ||
self.status_since = SystemTime::now(); | ||
|
||
vec![] | ||
}, | ||
(state, ProtocolEvent::Connected(peer_id)) => { |
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 all moved to handle_stats
due to these events not existing anymore: https://github.com/radicle-dev/radicle-upstream/pull/1463/files#diff-463e58bd670bb12315d14a2f26fbfc88e97a22551753a25ecd20fe3056fe9de3R414
proxy/coco/src/state.rs
Outdated
@@ -1,799 +1,878 @@ | |||
//! Utility to work with the peer api of librad. | |||
|
|||
use std::{convert::TryFrom as _, net::SocketAddr, path::PathBuf, sync::Arc, time::Duration}; | |||
use std::{convert::TryFrom as _, net::SocketAddr, path::PathBuf}; |
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 file easily has the worst diff to review. We removed the State
struct and made all the functions top-level. They now take &net::peer::Peer
to interact with the underlying Storage
. The changes were mechanical and the functions should be on par with what was there.
@@ -106,25 +106,6 @@ context("project checkout", () => { | |||
expect(result.stdout).to.equal(`rad`); | |||
} | |||
); | |||
|
|||
// Make sure we can't check out a project to the same directory twice. |
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.
Is this removed because the behavior changed?
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.
We saw that checkout
is idempotent, as confirmed by this test in working_copy.rs
https://github.com/radicle-dev/radicle-upstream/pull/1463/files#diff-940a798aab5c5c9a4058c1e8c8b384b600a22e234ece99249c8bd028a5be74dbR44
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 user facing behaviour has changed, and I think we should test this new behaviour. It's fine if we address this in a follow-up issue.
However, when testing the new behaviour manually, I bumped into some errors.
In a nutshell: when I create a project, and then re-add the same project again but with a different description, I get an error. Afterwards the project list screen also shows an error on every load.
Screen.Recording.2021-03-03.at.16.00.51.mov
{
"code": "ProjectCreationFailure",
"message": "Could not create project: the `rad` remote was found but the url field does not match the provided url, found: 'rad://hnrkps7zycckatezfsndcynur3f7jt5x94rdy.git' expected: 'rad://hnrkm8gpgi1koebhaaq965guh1193wysberto.git'",
"source": {
"response": {}
}
}
{
"code": "ProjectRequestFailure",
"message": "The project new could not be loaded",
"details": {
"type": "defaultBranch",
"urn": "rad:git:hnrkm8gpgi1koebhaaq965guh1193wysberto",
"shareableEntityIdentifier": "%rad:git:hnrkm8gpgi1koebhaaq965guh1193wysberto",
"metadata": {
"name": "new",
"description": "hola",
"defaultBranch": "main",
"maintainers": [
"rad:git:hnrkmndyrdnf17aspdu4ikm1s3adqwa16stby"
]
},
"stats": null
}
}
I also wonder if the new behaviour where adding an existing project just opens the project makes sense from a user's perspective. When re-adding the project, the notification says "Project created" although the project already exists, whereas before the UI would just show a validation error telling the user that the project already exists and blocking further action. \cc @juliendonck
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.
Thanks for the thoroughness @rudolfs 🙇 So what's happening is that the project with the same name and description has a different URN to the same name and no description. This is because they have different hash outputs. So, with this in mind, it's probably best that we don't allow to use the same working copy for checkout and guard against it.
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.
Actually, this is a bit more sinister than I thought. Before we were able to tell if an Entity
ended up with the same URN
. But we can only know the URN once we create the project, which is idempotent. So we can't easily tell if the project was created or just didn't do anything. Need to think about that more.
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.
Before we were able to tell if an
Entity
ended up with the sameURN
.
Wait what, how is this different now?
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.
Is it possible to determine the Urn
before calling create
? 🤔
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.
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.
Right, was just looking at that 😅 I'll create a proof for theory tomorrow
@@ -378,28 +378,6 @@ context("project creation", () => { | |||
.contains(`Project ${repoName} successfully created`) | |||
.should("exist"); | |||
commands.pick("notification").contains("Close").click(); | |||
|
|||
// Make sure we can't add the same project twice. |
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.
same question as above
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 built the release dmg package on my macOS Big Sur machine via yarn dist
and it seems to be broken due to some code signing issues in libraries linked in proxy. When I build the package on master, it just works, so something must have changed in our dependencies. 🤷
I don't have a Linux machine, but somebody should test our Linux AppImage package on this branch as well!
dyld: Library not loaded: /usr/local/opt/openssl@1.1/lib/libssl.1.1.dylib
Referenced from: /Applications/Radicle Upstream.app/Contents/Resources/radicle-proxy
Reason: no suitable image found. Did find:
/usr/local/opt/openssl@1.1/lib/libssl.1.1.dylib: code signature in (/usr/local/opt/openssl@1.1/lib/libssl.1.1.dylib) not valid for use in process using Library Validation: mapped file has no cdhash, completely unsigned? Code has to be at least ad-hoc signed.
@@ -106,25 +106,6 @@ context("project checkout", () => { | |||
expect(result.stdout).to.equal(`rad`); | |||
} | |||
); | |||
|
|||
// Make sure we can't check out a project to the same directory twice. |
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 user facing behaviour has changed, and I think we should test this new behaviour. It's fine if we address this in a follow-up issue.
However, when testing the new behaviour manually, I bumped into some errors.
In a nutshell: when I create a project, and then re-add the same project again but with a different description, I get an error. Afterwards the project list screen also shows an error on every load.
Screen.Recording.2021-03-03.at.16.00.51.mov
{
"code": "ProjectCreationFailure",
"message": "Could not create project: the `rad` remote was found but the url field does not match the provided url, found: 'rad://hnrkps7zycckatezfsndcynur3f7jt5x94rdy.git' expected: 'rad://hnrkm8gpgi1koebhaaq965guh1193wysberto.git'",
"source": {
"response": {}
}
}
{
"code": "ProjectRequestFailure",
"message": "The project new could not be loaded",
"details": {
"type": "defaultBranch",
"urn": "rad:git:hnrkm8gpgi1koebhaaq965guh1193wysberto",
"shareableEntityIdentifier": "%rad:git:hnrkm8gpgi1koebhaaq965guh1193wysberto",
"metadata": {
"name": "new",
"description": "hola",
"defaultBranch": "main",
"maintainers": [
"rad:git:hnrkmndyrdnf17aspdu4ikm1s3adqwa16stby"
]
},
"stats": null
}
}
I also wonder if the new behaviour where adding an existing project just opens the project makes sense from a user's perspective. When re-adding the project, the notification says "Project created" although the project already exists, whereas before the UI would just show a validation error telling the user that the project already exists and blocking further action. \cc @juliendonck
native/main.ts
Outdated
"--default-seed", | ||
"hynewpywqj6x4mxgj7sojhue3erucyexiyhobxx4du9w66hxhbfqbw@seedling.radicle.xyz:12345", | ||
]; | ||
proxyArgs = []; |
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.
Do we have a follow-up issue about setting up a production #next seed and updating the value here?
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.
It's in my brain stack
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’m about 70% through. Here are my (minor) comments so far.
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.
Some more things I discovered
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.
A couple more questions and suggestions.
/// # Errors | ||
/// * if initialisation of the repository fails | ||
/// * if branch or remote manipulation fails | ||
pub fn clone<F>( |
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 is not used outside the module
pub fn clone<F>( | |
fn clone<F>( |
There were some major changes in the radicle-link project that required radicle-upstream to update to in order for it to move forward on upcoming features. This patch introduces those changes and a short list of those changes are outlined below. The [Identity specification](https://github.com/radicle-dev/radicle-link/blob/a8f79e4b557030c07e64f7cfb299c000c754c751/docs/spec/sections/002-identities/index.md) was introduced and implemented. This replaces the Entity code, using Project, Person, and LocalIdentity. This is a breaking change that means the Radicle IDs for all projects and persons are different to the previous Entity Radicle IDs. The local transport became safer and we can use an API supplied by radicle-link for safely interacting with working copies. There was also an overhaul of the networking stack in radicle-link. This required some of the protocol messages to be removed and/or changed. In turn radicle-upstream had to adapt to these changes. A bonus of this part of the upgrade was that the project could bump the version of the tokio dependency to the latest major version >1.0. New features were reliant on radicle-link's latest changes. Combining this with the long life of this branch, some other changes are included in this patch. We list these changes here as well. Profile support was added. This means that a person can have multiple subdirectories under their XDG_* directories. These are keyed by a UUID and allows the person to switch to different profiles manually. The first step in Ethereum attestations was added. This allows a radicle-link Person document to be extended with an Eth address that was claimed on the blockchain. Signed-off-by: Alexander Simmerl <a.simmerl@gmail.com> Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com> Signed-off-by: Igor Zuk <igor.zuk@protonmail.com> Signed-off-by: Merle Breitkreuz <merle@monadic.xyz> Signed-off-by: Rūdolfs Ošiņš <rudolfs@osins.org> Signed-off-by: Thomas Scholtes <thomas@monadic.xyz>
014b879
to
f0081b3
Compare
Closes #1236
Closes #1226
Edit: Closes #1163
Edit: Closes #1078