-
Notifications
You must be signed in to change notification settings - Fork 109
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
base: main
Are you sure you want to change the base?
Improved nodes.go structure #1438
Conversation
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 **
This proposal has some impressive thinking behind it and is a great improvement on current nodes list. |
seems like a good change to me 👍 |
@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. |
Update: build now succeeds. Syntax corrected for:
|
- fixed syntax - removed trailing /graphql from focus graphql service entry
Well done @StarGeezerPhil hopefuly this gets picked up soon by deso-protocol/reviewers |
Thanks @carry2web. Simple example of why and how this would be useful:
E.g. (using a example/simulated updated /get-app-state.nodes return: @diamondhands0 @lazynina please see on-chain post (https://diamondapp.com/posts/5f8d804ca58a6263d93d58fb4cf5ab3cc58d84bd9f2e091f04638832af1074cd) 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! 🔥 |
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) |
@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. |
@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:
If you would be happy to consider in principal, I'm happy to make the necessary updates in a single tidy PR. |
Fully agree with @StarGeezerPhil that this stop-gap solution will improve current state. |
@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. |
@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 👍 |
If you want a formal statement on SafetyNet standing: |
[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:
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)