Update constants/magic numbers

[jonbarrow]

Resolves #XXX

Changes:

Draft for now as I want to go through and add all the missing values, as well as update any existing ones that need it.

There's a lot of work going on in the common protocols lib that is using magic numbers, such as:

• https://github.com/PretendoNetwork/nex-protocols-common-go/blob/5a86458671ae9d6ed8723bb996aef550304b967e/globals/utils.go#L131
• https://github.com/PretendoNetwork/nex-protocols-common-go/blob/5a86458671ae9d6ed8723bb996aef550304b967e/matchmake-extension/get_playing_session.go#L18
• https://github.com/PretendoNetwork/nex-protocols-common-go/blob/5a86458671ae9d6ed8723bb996aef550304b967e/matchmake-extension/database/get_playing_session.go#L14
• https://github.com/PretendoNetwork/nex-protocols-common-go/blob/5a86458671ae9d6ed8723bb996aef550304b967e/matchmake-extension/database/get_playing_session.go#L56

This PR aims to add all the missing magic numbers/constants/enums so that this doesn't keep happening. Nearly all the data comes from https://github.com/tech-ticks/RTDXTools/, with some RE/guess work mixed in.

I would also like to discuss the possibility of moving towards maps for these enums/flags. Go doesn't have real enums, and that does make it a little awkward to use/reference them since it means we either risk name collisions for simple names (such as Nothing) or we end up with very long-winded names (such as AutoMatchmakeOptionRecordLastGIDForParticipationCheck). So far I have opted to be inaccurate in places and ALWAYS prefix a value with its type name, even if that wasn't the case officially, so I'm not concerned about accuracy here.

By moving to maps we can potentially use code like this:

AutoMatchmakeOption.None

Which is a bit cleaner imo.

We already treat some types this way, such as:

• https://github.com/PretendoNetwork/nex-protocols-go/blob/28beb74753b4201e9198de68331497ee6572ab80/notifications/notification_categories.go
• https://github.com/PretendoNetwork/nex-go/blob/b5858fa5b2b6513c7470118e024a3f60b8595b2c/result_codes.go

The only question then, if we do decide to do this, is what to do about the map/struct/type names. For example, do we forego the "real" names in places for code like this:

type MatchmakeOption uint32

type matchmakeOptions struct {
  None                               MatchmakeOption
  RecordLastGIDForParticipationCheck MatchmakeOption
  Reserved1                          MatchmakeOption
}

var MatchmakeOptions = matchmakeOptions{
	None:                               0,
	RecordLastGIDForParticipationCheck: 1,
	Reserved1:                          2,
}

This works, but we'd be accessing the data as MatchmakeOptions.None, where MatchmakeOptions is not technically the "real name" since that is being used up by the type MatchmakeOption

Also, we should discuss moving some of the struct field types to these known types now that we have them since it would make certain code a bit cleaner due to not needing to cast when comparing to constants

• [x] I have read and agreed to the Code of Conduct.
• [x] I have read and complied with the contributing guidelines.
• [ ] What I'm implementing was an approved issue.
• [ ] I have tested all of my changes.

[DaniElectra]

IMO I prefer using Go constants over maps for these magic numbers, since it allows for reading the underlying value directly on IDE and is in line with making idiomatic code, which is part of our goal https://github.com/PretendoNetwork/nex-go/issues/73 . It may not be the cleanest, but I think it integrates better

[jonbarrow]

IMO I prefer using Go constants over maps for these magic numbers, since it allows for reading the underlying value directly on IDE and is in line with making idiomatic code, which is part of our goal PretendoNetwork/nex-go#73 . It may not be the cleanest, but I think it integrates better

This is fair 👍 in that case I propose the following standard:

• Use the real name where at all possible
• If the real name is likely to have conflicts and/or be confusing to use (such as with the Nothing value of GatheringFlags) then we prefix the values for the type with the types name

Also do we want to go ahead and update the notifications types to be consistent with the rest of the protocols constants, especially now that we have the real names? It would be a breaking change but I've already started renaming certain things here anyway so this whole PR is breaking already

[DaniElectra]

All of that sounds good to me 👍

[jonbarrow]

@DaniElectra I did some playing around with some ideas for notifications, how does this look?

package main

import "fmt"

type NotificationEvents uint32
type subType uint32 // Exists strictly to limit the types of values that can be passed to NotificationEvents.Build

func (ne NotificationEvents) Build(subtype ...subType) NotificationEvents {
	category := ne * 1000

	if len(subtype) == 0 {
		return category
	}

	return category + NotificationEvents(subtype[0])
}

const (
	NotificationEventsParticipationEvent NotificationEvents = 3
)

type ParticipationEvents = subType

const (
	ParticipationEventsParticipate ParticipationEvents = 1
)

func main() {
	notificationTypeA := NotificationEventsParticipationEvent
	notificationTypeB := NotificationEventsParticipationEvent.Build()
	notificationTypeC := NotificationEventsParticipationEvent.Build(ParticipationEventsParticipate)

	fmt.Println(notificationTypeA) // 3
	fmt.Println(notificationTypeB) // 3000
	fmt.Println(notificationTypeC) // 3001
}

I think this helps clean up some of the notification handling, though I'm a bit iffy on the name Build?

[DaniElectra]

On the PR you say that:

// NotificationEvents represents the main category of a notification type.

But then on the Build function the return type is also NotificationEvents, and the subtype gets converted to NotificationEvents too for this. I think the return type should just be uint32 or something else, but the type conversions should be logical

[jonbarrow]

@DaniElectra I've updated the return type to just be types.UInt32 so it can be used directly on the struct it's intended for

I've also pushed the first test of setting these constants to struct fields directly. I chose AutoMatchmakeParam for this just because it's the simplest, but I wanted to get your feedback on it. I had to make some changes to how the Copy and Equals functions work to support this change, which breaks some consistency, but the logic I think should be safe? We aren't using pointers anymore here and our new types are real types, not structs, so we should be able to just use normal operators for simple fields? Maybe we should consider moving all simple fields to doing this, as it would avoid extra function hops and stuff

[DaniElectra]

I like that yeah! We could update every enum/flags parameter like this as it would integrate better in code

Maybe we should consider moving all simple fields to doing this, as it would avoid extra function hops and stuff

By this you mean updating every simple field (such as types.UInt32 to uint32)?

[jonbarrow]

Maybe we should consider moving all simple fields to doing this, as it would avoid extra function hops and stuff

By this you mean updating every simple field (such as types.UInt32 to uint32)?

No I meant like going from this:

func (amp AutoMatchmakeParam) Equals(o types.RVType) bool {
	if _, ok := o.(AutoMatchmakeParam); !ok {
		return false
	}

	other := o.(AutoMatchmakeParam)

	if amp.StructureVersion != other.StructureVersion {
		return false
	}

	if !amp.SourceMatchmakeSession.Equals(other.SourceMatchmakeSession) {
		return false
	}

	if !amp.AdditionalParticipants.Equals(other.AdditionalParticipants) {
		return false
	}

	if !amp.GIDForParticipationCheck.Equals(other.GIDForParticipationCheck) {
		return false
	}

	if amp.AutoMatchmakeOption != other.AutoMatchmakeOption {
		return false
	}

	if !amp.JoinMessage.Equals(other.JoinMessage) {
		return false
	}

	if !amp.ParticipationCount.Equals(other.ParticipationCount) {
		return false
	}

	if !amp.LstSearchCriteria.Equals(other.LstSearchCriteria) {
		return false
	}

	return amp.TargetGIDs.Equals(other.TargetGIDs)
}

to this:

func (amp AutoMatchmakeParam) Equals(o types.RVType) bool {
	if _, ok := o.(AutoMatchmakeParam); !ok {
		return false
	}

	other := o.(AutoMatchmakeParam)

	if amp.StructureVersion != other.StructureVersion {
		return false
	}

	if !amp.SourceMatchmakeSession.Equals(other.SourceMatchmakeSession) {
		return false
	}

	if !slices.Equal(amp.AdditionalParticipants, other.AdditionalParticipants) {
		return false
	}

	if amp.GIDForParticipationCheck != other.GIDForParticipationCheck {
		return false
	}

	if amp.AutoMatchmakeOption != other.AutoMatchmakeOption {
		return false
	}

	if amp.JoinMessage != other.JoinMessage {
		return false
	}

	if !amp.ParticipationCount != other.ParticipationCount {
		return false
	}

	if !slices.Equal(amp.LstSearchCriteria, other.LstSearchCriteria) {
		return false
	}

	return slices.Equal(amp.TargetGIDs, other.TargetGIDs)
}

The use of the Equals method was mostly useful before the types update, and now is only really useful when either dealing with a complex struct (like SourceMatchmakeSession here) or with an instance of a RVType where we don't know the underlying type at runtime

Since our simple types (UInt32, String, etc.) are all type definitions based on real, basic, types and not structs, we can make use of standard operators/methods on them in places like this

[jonbarrow]

@DaniElectra In MatchmakeSessionSearchCriteria, the m_MatchmakeSystemType field is handled as a string for some reason but the value is a normal integer enum. Should we change the type of MatchmakeSessionSearchCriteria.MatchmakeSystemType from types.String to constants.MatchmakeSystemType? This would make it slightly inaccurate to the NEX type, but would make for easier integrations

[DaniElectra]

Maybe we should consider moving all simple fields to doing this, as it would avoid extra function hops and stuff

By this you mean updating every simple field (such as types.UInt32 to uint32)?

No I meant like going from this:

func (amp AutoMatchmakeParam) Equals(o types.RVType) bool {
	if _, ok := o.(AutoMatchmakeParam); !ok {
		return false
	}

	other := o.(AutoMatchmakeParam)

	if amp.StructureVersion != other.StructureVersion {
		return false
	}

	if !amp.SourceMatchmakeSession.Equals(other.SourceMatchmakeSession) {
		return false
	}

	if !amp.AdditionalParticipants.Equals(other.AdditionalParticipants) {
		return false
	}

	if !amp.GIDForParticipationCheck.Equals(other.GIDForParticipationCheck) {
		return false
	}

	if amp.AutoMatchmakeOption != other.AutoMatchmakeOption {
		return false
	}

	if !amp.JoinMessage.Equals(other.JoinMessage) {
		return false
	}

	if !amp.ParticipationCount.Equals(other.ParticipationCount) {
		return false
	}

	if !amp.LstSearchCriteria.Equals(other.LstSearchCriteria) {
		return false
	}

	return amp.TargetGIDs.Equals(other.TargetGIDs)
}

to this:

func (amp AutoMatchmakeParam) Equals(o types.RVType) bool {
	if _, ok := o.(AutoMatchmakeParam); !ok {
		return false
	}

	other := o.(AutoMatchmakeParam)

	if amp.StructureVersion != other.StructureVersion {
		return false
	}

	if !amp.SourceMatchmakeSession.Equals(other.SourceMatchmakeSession) {
		return false
	}

	if !slices.Equal(amp.AdditionalParticipants, other.AdditionalParticipants) {
		return false
	}

	if amp.GIDForParticipationCheck != other.GIDForParticipationCheck {
		return false
	}

	if amp.AutoMatchmakeOption != other.AutoMatchmakeOption {
		return false
	}

	if amp.JoinMessage != other.JoinMessage {
		return false
	}

	if !amp.ParticipationCount != other.ParticipationCount {
		return false
	}

	if !slices.Equal(amp.LstSearchCriteria, other.LstSearchCriteria) {
		return false
	}

	return slices.Equal(amp.TargetGIDs, other.TargetGIDs)
}

The use of the Equals method was mostly useful before the types update, and now is only really useful when either dealing with a complex struct (like SourceMatchmakeSession here) or with an instance of a RVType where we don't know the underlying type at runtime

Since our simple types (UInt32, String, etc.) are all type definitions based on real, basic, types and not structs, we can make use of standard operators/methods on them in places like this

Ohh okay, that sounds good to me!

In MatchmakeSessionSearchCriteria, the m_MatchmakeSystemType field is handled as a string for some reason but the value is a normal integer enum

That's because it is a string on MatchmakeSessionSearchCriteria, so that it's an optional parameter when searching

[jonbarrow]

In MatchmakeSessionSearchCriteria, the m_MatchmakeSystemType field is handled as a string for some reason but the value is a normal integer enum

That's because it is a string on MatchmakeSessionSearchCriteria, so that it's an optional parameter when searching

I see. How do you want to handle that then? That makes it rather annoying since it would be nice to be able to reuse the existing constant here, but we'd need to handle cases where it doesn't exist. MatchmakeSystemTypeInvalid exists but it's a "valid" type despite the name, it's not the "doesn't exist" type we'd need

Maybe we can just add a custom value to the enum that indicates if the value is missing or not?

[DaniElectra]

I think it can remain as it is, allowing us to handle it as we consider on the common protocols. We have to parse the value somewhere, and I'd prefer to do it in the common protocols than adding a fictional enum value just to parse it here

Also important note before we forget, we don't want to add validation checks for every field. For example, CTGP-7 modifies the m_MatchmakeSystemType field (not the m_GameMode) on the MatchmakeSession to a custom "invalid" value to avoid matching with vanilla users. This worked on Nintendo servers, so I believe that behavior should remain the same on our servers too

[jonbarrow]

Also important note before we forget, we don't want to add validation checks for every field. For example, CTGP-7 modifies the m_MatchmakeSystemType field (not the m_GameMode) on the MatchmakeSession to a custom "invalid" value to avoid matching with vanilla users. This worked on Nintendo servers, so I believe that behavior should remain the same on our servers too

I can opt to just remove the validation checks entirely if you think that would be better. We can handle all of that on the receiving end if need be?

I've already removed validation in one place in match making (not yet pushed) because I started using a real type alias which made it impossible to add multiple IsValid methods to subtypes, opting to just let the RMC handler do it, so maybe that should be done everywhere?

[DaniElectra]

Also important note before we forget, we don't want to add validation checks for every field. For example, CTGP-7 modifies the m_MatchmakeSystemType field (not the m_GameMode) on the MatchmakeSession to a custom "invalid" value to avoid matching with vanilla users. This worked on Nintendo servers, so I believe that behavior should remain the same on our servers too

I can opt to just remove the validation checks entirely if you think that would be better. We can handle all of that on the receiving end if need be?

I've already removed validation in one place in match making (not yet pushed) because I started using a real type alias which made it impossible to add multiple IsValid methods to subtypes, opting to just let the RMC handler do it, so maybe that should be done everywhere?

That sounds good to me :+1:

[jonbarrow]

(rebased the branch with the latest changes)

[jonbarrow]

I believe this is ready for review now. I don't THINK I missed anything? All the protocols which have known constants should have them now, I think, as well as using those constants in the protocols themselves

If I missed any constants do let me know

Also I wanted to ask: there's a couple values in Ranking2 which don't have constants, but might benefit from having them? Such as Ranking2CategorySetting.ResetMonth, which seems to be a 12-bit flag set for enabling months to reset seasons on, and Ranking2CategorySetting.ResetDay which denotes the day of the week/month to reset seasons on

It might be useful to create bespoke types for these, despite them not having true types? Let me know what you think @DaniElectra after you go through them, I'll be happy to add any bespoke types to whatever protocol if you think it's helpful

[jonbarrow]

Actually I lied, I missed the Nintendo variant of the notifications protocol. How do we want to handle that? We have no known names for any of those constants, just make something up I guess?

[jonbarrow]

You mention day of the week/month, does this mean that these two cases are represented under the same field.

Correct. Depending on the reset mode used, the resetDay value can either correspond to a day of the week (in the weekly reset mode) or a calendar day in an enabled month (in the monthly reset mode)

There will certainly be some overlap with values (for example I believe the weekday mode uses values 0-6, whereas the calendar days use 1-28, so the "2nd of the month" constant and "Wednesday" constant will both be value 2) but I don't think this would be an issue given that you also have the context of what mode is being used

[DaniElectra]

Also since I saw you fixed GetObjectInfos, reminder that ResetRatings also needs to be fixed

There will certainly be some overlap with values (for example I believe the weekday mode uses values 0-6, whereas the calendar days use 1-28, so the "2nd of the month" constant and "Wednesday" constant will both be value 2) but I don't think this would be an issue given that you also have the context of what mode is being used

Yeah that seems fair enough

[jonbarrow]

Actually maybe this isn't ready for review yet? I just double checked and so far only the following protocols have had constants added to them:

• DataStore
• MatchMaking
• Messaging
• Notifications
• NintendoNotifications
• Ranking
• Ranking2
• Subscriber

That's only 9 of the 32 protocols we have implemented here currently. I'm sure there's others which could use these? Such as friends, stuff like the max number of friends you can have or the max length of a status message or something?

[DaniElectra]

I have gone through every single missing protocol available here (including non-NEX) listing some constants that could be added (those with question marks mean I'm not fully certain about them since they may be impossible to know or would be unused in normal circumstances). This is a non-exhaustive list and I may have missed other constants that could be added:

• AA User
  • Some maximum for the number of entries for ApplicationInfo lists?
• Account Management
  • Some maximum for the number of entries for lists?
  • Some maximum length for strings?
  • Some maximum length for ResultRange? (On FindByNameLike)
• Debug
  • Some maximum for the number of entries for the input list of PIDs?
  • Some limit on the number of API calls returned?
• Friends (3DS)
  • Limit for string lengths (MAC address, serial number, comment and game mode description)
  • Limit for buffer lengths (Mii, picture and application arg)
  • Change bit flags
  • Mii character sets
  • Maximum number of friends: 100
  • Limit for list sizes (probably same as max number of friends)
  • Relationship types
• Friends (Wii U)
  • Limit for string lengths (NNID, presence message, persistent notification, friend request message, status message)
  • Limit for buffer lengths (Mii, application data)
  • Change bit flags
  • Maximum number of friends: 100
  • Limit for list sizes (probably same as max number of friends)
• Friends
  • Some maximum for the number of entries for lists?
  • Some maximum length for strings?
  • Some maximum length for ResultRange? (On GetRelationships)
• Match Making
  • ParticipationPolicy 4: PasswordProtected. The PolicyArgument is set to the MD5 of a key given by the user
  • ParticipationPolicy 95: Probably related to AnybodyParticipationPolicyArgument
  • ParticipationPolicy 96: Related to MatchmakeSystemTypeInvite
• Matchmake Referee
  • Some maximum for the number of entries for lists
  • Some maximum length for buffer on MatchmakeRefereePersonalRoundResult
  • Limit on initialRatingValue?
• Messaging
  • Maximum length of StrSubject: 60 (doesn't throw an error if bigger, just capped by the server)
• NAT Traversal
  • Limit for StationURL lists
• Persistent Store
  • Some maximum length for strings and buffers?
• Ranking (Legacy)
  • Number of scores: 2
  • Constants for each known "result code". Can we categorize these?
  • Limit for common data buffer. Must be exactly 0x14 on NEX 1, unknown limit on NEX 2
  • Ranking order params
  • Limit for uploading multiple scores
  • Limit for other lists?
• Rating
  • Limit for RatingStats and values lists
  • Limit for report data, common data and token buffer
• Remote Log Device
  • Limit for log string?
• Secure Connection
  • Limit for StationURL lists: probably 2 (public and private)
  • Limit for report data and token size
• Storage Manager
  • Number of slots: 5 (0-indexed)
• Subscription
  • Limit for target lists and max number of subscription datas (GetActivePlayerSubscriptionData? IIRC some request fields represent number of datas to get)
  • Limit for subscription data size
  • Privacy levels?
• Utility
  • Limit for AssociateNexUniqueIdsWithMyPrincipalId

[jonbarrow]

I have gone through every single missing protocol available here (including non-NEX) listing some constants that could be added (those with question marks mean I'm not fully certain about them since they may be impossible to know or would be unused in normal circumstances). This is a non-exhaustive list and I may have missed other constants that could be added:

• AA User

  • Some maximum for the number of entries for ApplicationInfo lists?

• Account Management

  • Some maximum for the number of entries for lists?
  • Some maximum length for strings?
  • Some maximum length for ResultRange? (On FindByNameLike)

• Debug

  • Some maximum for the number of entries for the input list of PIDs?
  • Some limit on the number of API calls returned?

• Friends (3DS)

  • Limit for string lengths (MAC address, serial number, comment and game mode description)
  • Limit for buffer lengths (Mii, picture and application arg)
  • Change bit flags
  • Mii character sets
  • Maximum number of friends: 100
  • Limit for list sizes (probably same as max number of friends)
  • Relationship types

• Friends (Wii U)

  • Limit for string lengths (NNID, presence message, persistent notification, friend request message, status message)
  • Limit for buffer lengths (Mii, application data)
  • Change bit flags
  • Maximum number of friends: 100
  • Limit for list sizes (probably same as max number of friends)

• Friends

  • Some maximum for the number of entries for lists?
  • Some maximum length for strings?
  • Some maximum length for ResultRange? (On GetRelationships)

• Match Making

  • ParticipationPolicy 4: PasswordProtected. The PolicyArgument is set to the MD5 of a key given by the user
  • ParticipationPolicy 95: Probably related to AnybodyParticipationPolicyArgument
  • ParticipationPolicy 96: Related to MatchmakeSystemTypeInvite

• Matchmake Referee

  • Some maximum for the number of entries for lists
  • Some maximum length for buffer on MatchmakeRefereePersonalRoundResult
  • Limit on initialRatingValue?

• Messaging

  • Maximum length of StrSubject: 60 (doesn't throw an error if bigger, just capped by the server)

• NAT Traversal

  • Limit for StationURL lists

• Persistent Store

  • Some maximum length for strings and buffers?

• Ranking (Legacy)

  • Number of scores: 2
  • Constants for each known "result code". Can we categorize these?
  • Limit for common data buffer. Must be exactly 0x14 on NEX 1, unknown limit on NEX 2
  • Ranking order params
  • Limit for uploading multiple scores
  • Limit for other lists?

• Rating

  • Limit for RatingStats and values lists
  • Limit for report data, common data and token buffer

• Remote Log Device

  • Limit for log string?

• Secure Connection

  • Limit for StationURL lists: probably 2 (public and private)
  • Limit for report data and token size

• Storage Manager

  • Number of slots: 5 (0-indexed)

• Subscription

  • Limit for target lists and max number of subscription datas (GetActivePlayerSubscriptionData? IIRC some request fields represent number of datas to get)
  • Limit for subscription data size
  • Privacy levels?

• Utility

  • Limit for AssociateNexUniqueIdsWithMyPrincipalId

Thank you for the list! I'm wondering how we should go about getting these values. Some of them like friends and such should be doable since that server is still online, but I'm unsure about the others?

Do the clients have local checks for these maybe? Or maybe we would be just need to resort to making stuff up and fixing mistakes as we go along

[DaniElectra]

Do the clients have local checks for these maybe? Or maybe we would be just need to resort to making stuff up and fixing mistakes as we go along

Some of them do have checks (like Messaging) but otherwise we will have to make it up