-
Notifications
You must be signed in to change notification settings - Fork 312
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
Effects typing fixes #947
base: master
Are you sure you want to change the base?
Effects typing fixes #947
Conversation
type
field
type
fieldtype_i: EffectType.account_credited; | ||
amount: string; | ||
} | ||
export interface AccountDebited extends BaseEffectRecord { | ||
export interface AccountDebited extends BaseEffectRecord<'account_debited'>, OfferAsset { |
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.
Also took the opportunity to fix this, AccountDebited
has OfferAsset
fields.
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.
Nice, I like it, Picasso 🤌 thank you!
@eumemic apologies for taking so long to review this - do you think you can sign the commits to get this merged? The repo blocks those commits otherwise. |
Parameterize
BaseEffectRecord
by effect type literals so that it's possible to discriminate based on thetype
field. With this change code like the following type checks whereas before it would have had errors because the type ofeffect
wouldn't get appropriately narrowed in each case based on the value ofeffect.type
:Also added the
type_i
field to theTrade
interface since it was missing, which was preventing eventype_i
from working correctly to discriminate effect types.