search

PretendoNetwork / nex-protocols-common-go

Reusable implementations of NEX methods found in many servers
GNU Affero General Public License v3.0
19 stars 8 forks source link

Add common DataStore protocol #24

Closed jonbarrow closed 1 year ago

jonbarrow commented 1 year ago

Lays the ground work for implementing a common DataStore protocol. This one is going to be very heavy on helper methods just due to the nature of how DataStore needs to operate

So far this implements enough for Super Mario Maker, every stock DataStore method I've seen the game use is implemented here. All methods seen in Super Mario Maker have been heavily tested using NintendoClients to verify the functionality against the real server including many edge cases, and I have tried to leave detailed comments in areas that may have confusing handling

Note that this is far from a complete implementation at this time. Some additional methods outside of the ones seen in Super Mario Maker are also implemented, however they are ported from existing servers which use DataStore. These servers are older and the functionality of these methods has not been 100% tested, but it all seems correct

One thing we should also decide on now is how to handle S3. For several methods we need access to the servers S3 bucket. This is currently just set using SetS3Bucket. However we also have the option of just having the user pass some configuration parameters and then the common library creates and manages the S3 client. Originally this would not be done in order to give the developer as much control as possible. However this will lead to some, honestly unnecessary, duplicated code between server implementations. The same initialization and helper methods will be defined in basically all servers. And with S3 options now extremely limited, I'm not positive this is a worthwhile thing to support. I'm not a fan of vendor lock-in, but when it comes to DataStore there isn't much of a choice now besides MinIO. Trying to use the major cloud providers is no longer tenable, and I plan to recommend the use of MinIO in all servers which use DataStore in the repo's README anyway, plus the Super Mario Maker server already just uses the MinIO SDK

Moving the S3 client to common (and providing a getter for it for game-specific stuff) would remove the need to set several helper methods as well, however I understand if there is opposition to this idea as it DOES enter the realm of vendor lock-in

I do not think we should control the database layer though, the proposal is only for the S3 layer. We make use of Postgres to keep things uniform across our servers, and that code will be duplicated, but there is a legitimate use case for not controlling that layer here. Other developers are not limited in terms of database solutions as they are with S3 options, so anyone wanting to fork and rehost our servers should always be allowed to swap out Postgres with whatever database solution they use so long as the helper functions are implemented correctly

Currently implements

jonbarrow commented 1 year ago

Marking this as ready for review now because all DataStore methods that I have personally seen in servers has been implemented

Since this PR comes from a branch on this repo though please feel free to push any new methods for games I have not touched

jonbarrow commented 1 year ago

Using the MinIO SDK shouldn't affect implementations that want to use another server since they all use S3

Oh perfect. The MinIO docs have separate examples for when connecting to MinIO vs AWS so I assumed the MinIO client could only connect to MinIO servers. Awesome

In order to allow custom configurations, I think we could let the server implementation create the MinIO client and pass it to the common protocols

This works for me. I was originally thinking about just providing a custom struct the user could fill their config details into and then common would create the client. But that was somewhat naive, and ignored advanced configurations. This is the better solution

we should consider integrating HPP here aswell

I agree. As far as I know HPP should be, for servers which support it, a 1:1 substitute for NEX, and any protocols provided by NEX would be available to the HPP client (correct me if I'm wrong there). So it being supported here makes sense

What all work needs to be done for this? I haven't looked much into HPP, even the implementation in nex-go, so I'm unsure what changes need to be made to bring support here

MK7 will use V1 versions of methods

Over HPP or over NEX? Either way those should be implemented here tbh, we can add these to this PR if we have working implemenations of them. Just push to the branch

On all HPP requests I've seen, the flags of PreparePostParamV1 is always 0x8, so I think we could check for that when handling notifications (unless we find a game that uses the flag outside of HPP)

I'm not positive that's what this flag means, but it sounds like a good assumption for now. SMM doesn't use this flag for it's objects. Objects in SMM only have flags of 0, 256, or 3840 from what I can tell and I have no idea what they mean. We should make a note in the implementation though that this is basically just a heuristics guess and the exact flag use is not known

DaniElectra commented 1 year ago

What all work needs to be done for this? I haven't looked much into HPP, even the implementation in nex-go, so I'm unsure what changes need to be made to bring support here

I was thinking about this and I think we can't differentiate a NEX packet from an HPP packet right now? On nex-protocols-go we are checking the packet type, but we don't have access to that here (and there is no param on the server right now that differentiates one from another). I think we could add a boolean on nex-go to check this?

Aside from this, I was thinking that we don't have access to the packet source and destination when we reach a method. We will need to look into this when we want to implement RV.

Over HPP or over NEX? Either way those should be implemented here tbh, we can add these to this PR if we have working implemenations of them. Just push to the branch

This is over NEX on PRUDP, Mario Kart 7 uses methods like PreparePostObjectV1 working the same as the normal method.

I'm not positive that's what this flag means, but it sounds like a good assumption for now. SMM doesn't use this flag for it's objects. Objects in SMM only have flags of 0, 256, or 3840 from what I can tell and I have no idea what they mean. We should make a note in the implementation though that this is basically just a heuristics guess and the exact flag use is not known

Sure, we can add a note about this

jonbarrow commented 1 year ago

Aside from this, I was thinking that we don't have access to the packet source and destination when we reach a method. We will need to look into this when we want to implement RV.

I was also thinking about this the other day. Easiest solution would probably be to just send the original packet to the method instead of the client. The client can be obtained from the packets Sender() method instead, and then we have full access to all the context we've been missing

This is over NEX on PRUDP, Mario Kart 7 uses methods like PreparePostObjectV1 working the same as the normal method.

Gotcha gotcha

jonbarrow commented 1 year ago

@DaniElectra @shutterbug2000 How are we feeling about the proposal to stop sending the client in the protocol handlers and instead sending the original packet? We can still get the client using the packet's Sender() method, and it would allow us to:

I know it would make the function signatures look a bit goofy, but I'm thinking it's a worthwhile change considering the benefits

DaniElectra commented 1 year ago

How are we feeling about the proposal to stop sending the client in the protocol handlers and instead sending the original packet? We can still get the client using the packet's Sender() method, and it would allow us to:

* Seamlessly support HPP from what I can tell, with no further changes to either the protocols or common libs

* Stop hard-coding the destination and source addresses in preparation for QRV support

I know it would make the function signatures look a bit goofy, but I'm thinking it's a worthwhile change considering the benefits

I agree. Go with it!

jonbarrow commented 1 year ago

How are we feeling about the proposal to stop sending the client in the protocol handlers and instead sending the original packet? We can still get the client using the packet's Sender() method, and it would allow us to:

* Seamlessly support HPP from what I can tell, with no further changes to either the protocols or common libs

* Stop hard-coding the destination and source addresses in preparation for QRV support

I know it would make the function signatures look a bit goofy, but I'm thinking it's a worthwhile change considering the benefits

I agree. Go with it!

Awesome, I'll get the changes made then! I don't think it's entirely relevant to this PR however so I'll make another one after this PR (seeing as all the protocols here will need updating)

For now, is there anything else left before we merge this?