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

Improved nodes.go structure #1438

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

StarGeezerPhil
Copy link

@StarGeezerPhil StarGeezerPhil commented Dec 23, 2024

[UPDATED for clarity/updates]

Summary: an improved structure to make '/get-app-state' more useful for live services or product catalogue listing (e.g. "Apps on DESO"), currently there's numerous issues - the entries are mostly inactive, most only list the front-end location - some could have the API located elsewhere, may only be a front-end and not have a backend service or may even provide graphQL services.

** Note: I have retained all existing keys retained to avoid breaking changes **

Updated structure aims to make the list suitable for easily identifying active/inactive status of nodes, apps and graphQL services - with three URL keys clearly indicating what service(s) each entry provides. The existing URL key has been retained to avoid breaking changes as is to specify the location of the backend service (if provided).

New keys to clarify:

  • Active - boolean whether entry is active (for history and active service lookups)
  • URL endpoints for service(s) provided:- (presence/absence indicates service provision, marked as optional fields)
    • URL (IF backend/API service is provided)
    • FrontendURL (IF frontend service is provided)
    • GraphqlURL (IF graphQL service is provided)
  • EntryPublicKeyBase58Check and OwnerPublicKeyBase58Check (consistency over username, provide profile & avatar for listings)

Fully updated with active statuses and known changes to URLs and service availability.

@lazynina could this also help in providing a trusted and active node list for backend init stages (before P2P discovery)?

@nader / @diamondhands0 - please also see the comments at the end - there is some significant usage of key 30 already, I don't know whether you may wish to update these keys before focus is launched into production.

If you're happy with this proposal in principal I'm happy to update renumbering 30 onwards chronologically.

Example catalogue listing using a simulated app state data return - #1438 (comment)

Improved structure to make '/get-app-state' more useful for live services.
May also provide trusted active node list for backend init.
Updated with active statuses and known changes to URLs and service availability.
** Note: existing keys retained to avoid breaking changes **
@StarGeezerPhil StarGeezerPhil requested a review from a team as a code owner December 23, 2024 09:16
@carry2web
Copy link

This proposal has some impressive thinking behind it and is a great improvement on current nodes list.

@matt-mcmarsh
Copy link

seems like a good change to me 👍

@StarGeezerPhil
Copy link
Author

@superzordon @diamondhands0 notice this failed on build, but (apologies) - I'm not familiar with go and there's no debug return to indicate the cause - could be as simple as my comments or the position of them?

If it's possible to add this additional info, I know it'd be very useful for all sorts of applications on the blockchain (unless we can introduce a live/on-chain record???).

Happy to refactor if anyone can highlight what causes the noted build fail.

@StarGeezerPhil
Copy link
Author

Update: build now succeeds.

Syntax corrected for:

  • comments (use of /* ... */ in additional node comments at end)
  • *string replaced with standard string for the optional URL parameters (should assume blank if key not specified)

- fixed syntax
- removed trailing /graphql from focus graphql service entry
@carry2web
Copy link

carry2web commented Dec 24, 2024

Well done @StarGeezerPhil hopefuly this gets picked up soon by deso-protocol/reviewers

@StarGeezerPhil
Copy link
Author

Thanks @carry2web.

Simple example of why and how this would be useful:

  • live listing of active API services (e.g. monitoring tools)
  • catalogue/live listing of apps/front-ends on the deso network
  • access to active graphql services across the network

E.g. (using a example/simulated updated /get-app-state.nodes return:
image

@diamondhands0 @lazynina please see on-chain post (https://diamondapp.com/posts/5f8d804ca58a6263d93d58fb4cf5ab3cc58d84bd9f2e091f04638832af1074cd)
...in reference to...

Prior proposals that were skipped:

I'm happy to update this rewrite in chronological order so that Nathan's much earlier request can be honoured for node ID 30, which he has been using for some considerable time already. Preventing a mess with Focus when it launches into production.

This would push Focus to 31, SafetyNet to 32 and so on.

Please let me/us know team Deso 💖

Come on 2025! 🔥

@StarGeezerPhil
Copy link
Author

Further thought:

Might be more logical for URL to represent the front-end location (as most currently are as such) - dropping "FrontendURL"

Instead, add a "BackendURL" key for the API/backend locations instead.

Happy to update with a revised commit (or submit as a all-in-one tidied PR with this and the chronological ID fixes if that's more suitable)

@lazynina
Copy link
Member

lazynina commented Jan 3, 2025

@StarGeezerPhil - thanks for proposing a new structure to nodes.go. While there are some interesting and useful enhancements here, the use of nodes.go was originally meant only for post attribution and it's not exactly a particularly great solution for that either. In the long run, we hope to re-assess this file and replace with some on-chain functionality as opposed to a file that lives in our core repo. We don't want to need to cut releases when a new node is added. The overlap between different nodes trying to take the same ID causes problems.

We'd love to have a more decentralized solution to post attribution as well as service discovery instead of the current solution. Unfortunately addressing this is a low priority for now and we won't be considering changes imminently.

@StarGeezerPhil
Copy link
Author

StarGeezerPhil commented Jan 3, 2025

@lazynina thank you for reviewing and I agree completely that an on-chain, decentralised, solution would be much better... but, in the meantime the front-ends still need to be able to reference each other, whilst you want to avoid the duplication on the node 30 key.

As a minimum, could we have the PRs for MyDesoSpace (30) and SafetyNet approved, with focus moved to 31 and SafetyNet to 32 (chronologically, and avoiding mass duplication on 30 - currently 4 apps/nodes).

However, assuming this does not introduce breaking changes could you consider implementing the improvements as a stop gap as there is no harm - I am sure others would use this as well, but it has immediate benefits:

  • the deso website itself - listing active projects (as I'm sure you're aware, many if not most are now dead projects)
  • API access and network status listing (easy for any front-end to list and query active backends)
  • third party (non-deso website) app listings/marketplace

If you would be happy to consider in principal, I'm happy to make the necessary updates in a single tidy PR.

@carry2web
Copy link

Fully agree with @StarGeezerPhil that this stop-gap solution will improve current state.
The future direction should indeed be fully on-chain and dynamic, but until then the proposed PR will improve things considerably.

@lazynina
Copy link
Member

lazynina commented Jan 3, 2025

@StarGeezerPhil - Focus will retain node ID 30. Apps should not expect to have a given node ID unless the code is merged in.

I am awaiting a response from others on the team regarding SafetyNet's PR.

I will ask the others on the team another time if they wish to consider these changes, but the current opinion is that this type of change will not be accepted at this time.

@StarGeezerPhil
Copy link
Author

@lazynina thanks - appreciate that.

I think (in fairness) he needed to get going and hadn't received any update on the PR (but can't speak on his behalf). While, I'd suggested it so you didn't have any other "noise" on the Focus key.

I'd hoped with some needing to be added anyway we might be able to get it all pushed through in a single update (like you say - avoiding repeated backend image updates). Really appreciate it if we can - I'll be using this myself anyway and just wanted to help other operators at the same time 👍💖

Just let me know and I'll get everything tidied up without any delay if it can be pushed 👍

@carry2web
Copy link

If you want a formal statement on SafetyNet standing:
We fully support @StarGeezerPhil suggested numbering:
30 MyDeSoSpace (Nathan was first)
31 Focus (can still be done, launch pending)
32 SafetyNet

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.

4 participants