Skip to content
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

feat: show full rpc backend version #1294

Merged
merged 4 commits into from
Sep 25, 2023
Merged

Conversation

lidel
Copy link
Member

@lidel lidel commented Sep 22, 2023

This PR will try to read version from ipfs.id before it falls back to ipfs.version.

Rationale:

  • we now have useful AgentVersion returned by 'ipfs id' Kubo RPC command which allows for including suffix (e.g. in brave).
  • this makes it more obvious that kubo backend is used, and in which version, and removes perception that kubo version === ipfs version

Preview

Before After
2023-09-22_15-21 2023-09-22_15-22 2023-09-22_15-25

we now have useful AgentVersion returned by 'ipfs id' Kubo RPC command
which allows for including suffix (e.g. in brave).

this makes it more obvious that kubo backend is used, and in which
version, and removes perception that kubo version === ipfs version
@lidel lidel requested review from whizzzkid and a team as code owners September 22, 2023 13:30
@@ -35,13 +35,9 @@
"message": "The URL of your local Kubo RPC",
"description": "A label in Node status section of Browser Action pop-up (panel_statusApiAddressTitle)"
},
"panel_statusGatewayVersion": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ this was not used anywhere

@@ -27,7 +27,7 @@ export default (state, emitter) => {
publicSubdomainGatewayUrl: null,
gatewayAddress: null,
swarmPeers: null,
gatewayVersion: null,
kuboRpcBackendVersion: null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ renamed to make it clear this is version of RPC backend, and not gateway.
(if we want versioned gateways, we should expose gateway-conformance version, but this is future work)

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but we should give @whizzzkid a chance to review before merging

Copy link
Contributor

@whizzzkid whizzzkid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's a good idea to hard code this to kubo even though it's true for now, we want this to work with any ipfs implementation in the future?

also, nits!

Comment on lines 235 to 250
try {
const id = await ipfs.id()
if (id) {
return id.agentVersion
}
} catch (_) {
try {
const v = await ipfs.version()
if (v) {
return v.commit ? v.version + '/' + v.commit : v.version
}
} catch (_) {
}
}
return null
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❗ woah

Suggested change
try {
const id = await ipfs.id()
if (id) {
return id.agentVersion
}
} catch (_) {
try {
const v = await ipfs.version()
if (v) {
return v.commit ? v.version + '/' + v.commit : v.version
}
} catch (_) {
}
}
return null
}
try {
const { agentVersion } = await ipfs.id()
if (agentVersion) {
return agentVersion
}
const {version, commit} = await ipfs.version()
return [version, commit].filter(Boolean).join('/')
} catch (_) {}
return null
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, applied in 90f32c8

@lidel
Copy link
Member Author

lidel commented Sep 23, 2023

we want this to work with any ipfs implementation in the future?

Good question @whizzzkid!
Here is how I think about this, in the context of effort to ensure "Kubo !== IPFS", but just an implementation.

We want Companion to work with other backends, yes, but currently, the default backend is Kubo-specific, and we should start making it more obvious in the code (makes it easier to refactor in the future, when we add different backends).

The /api/v0 is "Kubo RPC" and not "IPFS API" for a reason.
We've invested a lot of time to remove confusion that Kubo is not "IPFS" and that /api/v0 is not "IPFS API".

Clearly naming Kubo-specific things makes it easier to understand there the line between "generic IPFS" like HTTP Gateways on /ipfs and /ipns or /routing/v1 and "legacy/kubo-specific" like /api/v0.

Other implementations are free to implement endpoint compatible with Kubo RPC and become drop-in replacement (ipfs-cluster does this already), but it is still called "Kubo RPC" as Kubo is the source of truth, and the API is defined at https://docs.ipfs.tech/reference/kubo/rpc/, and has utility limited by Kubo itself (we talk to RPC using a dedicated kubo-rpc-client, we make breaking changes to this RPC API and there is no guarantee of stability nor being "general-purpose", it is not part of https://specs.ipfs.tech).

I think what we want to do over time is to make it more clear that "External" backend today is Kubo-specific, and create room for other implementations and their own RPC protocols (if they decide to use something other than Kubo RPC compatible one).

I did nto want to rename other variables to keep this PR easier to review, but I did rename label to "External (Kubo RPC)" in eac436c to make it extra clear to the user that this is "Kubo-compatible" backend.

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
@whizzzkid whizzzkid merged commit 9f852ef into main Sep 25, 2023
5 checks passed
@whizzzkid whizzzkid deleted the feat/show-full-agent-version branch September 25, 2023 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants