Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Rest Server] stop/start/genesis endpoints #454
[Rest Server] stop/start/genesis endpoints #454
Changes from all commits
8fecd38
f829bb9
e47f771
a56d937
6789820
f2d713e
331af96
82d7c6e
2964cba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 brittle and somewhat panic-inducing: IIUC, if something goes wrong we blow up on a started server that the user has hit an endpoint of, not. on starting the server itself. I would at least open an issue for the following:
NetworkConfig
,GenesisConfig
, etc should be created at theServerContext
constructor.Moreover, all of this setup is re-done from scratch on every endpoint hit. Is there a reason why this should happen?
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 are only exposing these endpoints because we plan on hosting instances of the rest server for game developers. Which means if they wanted to reset the SUI network the rest server is using, they need to be able to do so using the endpoints. In the long term none of this will matter because we shouldn't be starting the Sui network from within the rest server anyways. I can open an issue discussing the future design of the rest server post-GDC.
Because the server context is instantiated once here, all other changes to the server context happen via the endpoints. There are checks in the endpoints to see if the network is already running or the configs are already created.
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.
What's wrong with having, as we seem to, one endpoint that starts the network and calls the constructor of the server's state, one that tears it down? It would be a contract with our early test developers that nothing interesting is going to happen if they don't hit that particular bootstrap first, and that they'll carry over the networks' state if they don't tear it down before a restart.
Then we can have real state, with configuration that contains actual data rather than recreating everything on the fly.
My issue isn't with the re-startability it's with the multiplication of panic-prone config read and scaffolding code in each endpoint handler.
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 seems we're maintaining a copy of this in
sui_commands
, calledstart_network
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 am handling the async portion of this differently than
start_network
does. I am maintaining a hold of theJoinHandle
so that I can abort it later during start. Don't think reusingsui_commands
makes sense here, but correct me if I am wrong.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.
Fair enough. Additionally, as we've seen, the pattern of what's going on in
start_network
andgenesis
insui_commands
doesn't make sense: the task waits on each launched server to finish (rather than wait for each server to start) here:https://github.com/MystenLabs/fastnft/blob/dba291bfac0d9a9811e9a296f3f4712744ef2df6/sui/src/sui_commands.rs#L86-L88
As we've seen, that's not really usable in a task, which should keep a handle rather than block on completion.
Would you and @patrickkuo open an issue to track the refactoring of
start_network
in a way that:SpawnedServer
rather blocking on it,sui_commands
?