-
Notifications
You must be signed in to change notification settings - Fork 8
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
add events dummy example to chess #84
base: main
Are you sure you want to change the base?
Conversation
"author": "PaimaStudios", | ||
"dependencies": { | ||
"esrun": "^3.2.26", | ||
"mqtt": "5.8.1", |
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.
the client shouldn't need mqtt
is this some workaround for something not compiling?
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.
same as ``esrun ?
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.
yeah, I forgot I did this. Since I'm mostly just copying the dependencies locally I added this to install it, but normally it would be a transitive dependency.
"dependencies": { | ||
"esrun": "^3.2.26", | ||
"mqtt": "5.8.1", | ||
"@sinclair/typebox": "0.33.4" |
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.
If types need to be defined with typebox
perhaps it should be exposed in paima-engine
{ | ||
indexed: false, | ||
name: 'isPractice', | ||
type: Type.Boolean(), | ||
}, |
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 looks like v1 & v2 where defined to have this optional type Type.Optional(Type.Boolean())
It might be better for the insistence as both events have the same name and indexed fields (and there is no way of distinguishing them)
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.
They can be distinguished by the topic (which is at app/topic
). Since the topic is the hash of the signature, these will have different hashes, because one has one extra field.
But you could define a single event with the optional field as you say. The thing is that events are immutable, you can't add a field to an existing one, so this was an example of what you would need to do if you want to add a new field. So let's say you forgot to add the field initially, you can add this later for example.
Or if you want to change the type of an existing field you would need to do something similar.
chess/events/src/index.ts
Outdated
type: Type.Number(), | ||
}, | ||
], | ||
} as const); |
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.
as const required?
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 guess not, I probably added it when testing something.
chess/events/src/index.ts
Outdated
], | ||
}); | ||
|
||
export const MatchWon = genEvent({ |
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 should have the field lobbyId indexed, and winner not indexed
|
||
// Persist creation of a lobby | ||
export function persistLobbyCreation( | ||
player: WalletAddress, | ||
blockHeight: number, | ||
inputData: CreatedLobbyInput, | ||
randomnessGenerator: Prando | ||
randomnessGenerator: Prando, | ||
events: Events |
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 think we shouldn't return data by modifying the reference, it works - but as there is no way to tell the programmer this will happen (as in other languages that you can mark as mutable) in the worst case perhaps call it mutableEvents
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.
That's just how I prefer to write it though, there is nothing enforcing one style or the other. I personally always assume that if I pass an array then it may be mutated (in js). In this case there is not really any point in reading from this object, so it has to be write only, but that can be clearer through the name as you mention.
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 just a personal preference thing and it's up to each game to decide how they want to do this. This PR is more to decide if the interface of the things coming from Paima Engine itself is good or not
} | ||
default: | ||
return []; | ||
return { stateTransitions: [], events: [] }; |
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.
@ecioppettini just a question, this is deterministic, first all stateTransitions
get written in order, then events get sent?
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.
yes, if any of the state transition queries rollbacks then nothing gets emitted also.
|
||
if (results[0] !== 't') { | ||
events.push( | ||
encodeEventForStf(precompiles.default, MatchWon, { |
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 feel like this should probably have an interface that looks more like what I wrote below since I think this is a lot more clear what is going on
encodeEventForStf({
from: precompiles.default,
topic: MatchWon,
data: {
winner: results[0] === 'w' ? matchEnvironment.user1.wallet : matchEnvironment.user2.wallet,
}
});
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 updated the interface to look like this (PaimaStudios/paima-engine@63bb369).
This is not a functional change, just an example application of the current code in PaimaStudios/paima-engine#409 . It's also possible it may not compile, since I'm using local dependencies.
I defined an overloaded event to just to show how it looks, but both events are emitted at the same time (which could be a valid use-case though)
The events table looks something like this:
The joined lobby event was never emitted so it's not there.
The registered_event table looks like this:
The output of
mqtt subscribe -h 127.0.0.1 -p 8883 -q 2 -l ws -t '#' -v
looks something like this for example: