Skip to content
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

Feature/insert events for trade and fee #535

Conversation

taiki1frsh
Copy link
Collaborator

close #533

  • Define events that are emitted when the protocol earned (or lose) fee
  • Insert them
  • Add description in README

@taiki1frsh taiki1frsh requested review from mkXultra and jununifi May 17, 2023 04:28
(gogoproto.moretags) = "yaml:\"position_id\""
];
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code patch appears to be adding three protobuf messages that define fees related to perpetual future trading.
There are no obvious bug risks with this code and the additional fields are well-defined with appropriate tags. However, further context such as the intended use-case, and usage in other parts of the system could provide a more comprehensive review.

(gogoproto.moretags) = "yaml:\"position_id\""
];
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch is adding three new events related to fee earned by the protocol from traders in a derivatives trading platform. The events seem clear and straightforward, and the protobuf message definitions also look well-defined.

As for improvement suggestions, it may be useful to include more detailed information about the relevant parameters in the events, such as the size of the fees and how they are calculated, to make it easier for users to understand them without having to refer to external documentation.

Regarding risks, it would be worth verifying that there are no errors or inconsistencies in the calculations related to the fees or their distribution, as any issues in this regard could potentially lead to financial losses or other problems for traders using the platform.

_ = ctx.EventManager().EmitTypedEvent(&types.EventPerpetualFuturesImaginaryFundingFee{
Fee: sdk.NewCoin(position.RemainingMargin.Denom, imaginaryFundingFee.Neg()),
PositionId: position.Id,
})
}
position.LastLeviedAt = ctx.BlockTime()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch is emitting some custom events to the event manager. These emitted events are EventPerpetualFuturesTradingFee, EventPerpetualFuturesLiquidationFee, and EventPerpetualFuturesImaginaryFundingFee. This is a good practice for logging purposes and makes debugging easier.

Regarding bugs, there seem to be no evident bugs within this code patch. However, further analysis would require a broader view of the entire codebase.

As for an improvement suggestion: It seems like multiple calls emit the same event with different values. Consider refactoring these similar code lines and extracting them into helper methods.

(gogoproto.moretags) = "yaml:\"position_id\""
];
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code patch seems to add new features regarding events emitted by the protocol and their corresponding protobuf messages. There don't seem to be any major bug risks in this code, but some suggestions for improvement could include adding more detailed comments and explanations for each event and its purpose, as well as potentially adjusting field names or tags if they do not conform to best practices or existing conventions.

@Senna46 Senna46 merged commit 3c85e18 into feature/modify-query-properies Jun 5, 2023
@Senna46 Senna46 deleted the feature/insert-events-for-trade-and-fee branch June 5, 2023 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants