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

AMM-CDA-7 Add event information for indexer for Hybrid Orderbook AMM #1288

Closed
wants to merge 18 commits into from

Conversation

Chralt98
Copy link
Member

@Chralt98 Chralt98 commented Mar 14, 2024

What does it do?

It adds necessary event information for the indexer and front end.

It contains aggregated amounts for the asset out, the related AMM trades and order book trades plus an optional order amount information, if an order was placed at the end.

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

References

@Chralt98 Chralt98 added the s:blocked The pull requests awaits resolution of dependencies (state what it depends on) label Mar 14, 2024
@Chralt98 Chralt98 self-assigned this Mar 14, 2024
@mergify mergify bot added s:in-progress The pull requests is currently being worked on and removed s:blocked The pull requests awaits resolution of dependencies (state what it depends on) labels Mar 15, 2024
@Chralt98 Chralt98 changed the base branch from chralt98-amm-cda-event-compare to chralt98-amm-cda-release March 22, 2024 14:42
@Chralt98 Chralt98 changed the base branch from chralt98-amm-cda-release to chralt98-amm-cda-event-compare March 22, 2024 14:43
Copy link
Member

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

I think we should clarify what the fee info in the events means. The tests need to reflect the changes made here.

@@ -122,13 +125,27 @@ mod pallet {
{
/// A trade was executed.
HybridRouterExecuted {
/// The type of transaction (Buy or Sell).
Copy link
Member

Choose a reason for hiding this comment

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

Are the fee amounts contained in the amount_* values? This should be clarified in the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Chralt98 Chralt98 changed the base branch from chralt98-amm-cda-event-compare to chralt98-amm-cda-6-5 March 27, 2024 12:51
Copy link
Contributor

mergify bot commented Mar 27, 2024

This pull request is now in conflicts. Could you fix it @Chralt98? 🙏

@mergify mergify bot added s:revision-needed The pull requests must be revised s:in-progress The pull requests is currently being worked on and removed s:in-progress The pull requests is currently being worked on labels Mar 27, 2024
@mergify mergify bot removed the s:in-progress The pull requests is currently being worked on label Mar 27, 2024
@Chralt98 Chralt98 changed the base branch from chralt98-amm-cda-6-5 to chralt98-amm-cda-release March 27, 2024 13:12
@Chralt98 Chralt98 changed the base branch from chralt98-amm-cda-release to chralt98-amm-cda-6-5 March 27, 2024 13:12
@mergify mergify bot added s:in-progress The pull requests is currently being worked on s:revision-needed The pull requests must be revised and removed s:revision-needed The pull requests must be revised s:in-progress The pull requests is currently being worked on labels Mar 27, 2024
@mergify mergify bot added s:in-progress The pull requests is currently being worked on s:revision-needed The pull requests must be revised and removed s:revision-needed The pull requests must be revised s:in-progress The pull requests is currently being worked on labels Mar 28, 2024
@Chralt98 Chralt98 added s:in-progress The pull requests is currently being worked on and removed s:revision-needed The pull requests must be revised labels Mar 28, 2024
@mergify mergify bot added s:revision-needed The pull requests must be revised and removed s:in-progress The pull requests is currently being worked on labels Mar 28, 2024
@mergify mergify bot added s:in-progress The pull requests is currently being worked on s:revision-needed The pull requests must be revised and removed s:revision-needed The pull requests must be revised s:in-progress The pull requests is currently being worked on labels Mar 28, 2024
Base automatically changed from chralt98-amm-cda-6-5 to chralt98-amm-cda-release March 29, 2024 06:32
@Chralt98 Chralt98 changed the base branch from chralt98-amm-cda-release to main March 29, 2024 06:35
@Chralt98 Chralt98 changed the base branch from main to chralt98-amm-cda-release March 29, 2024 06:36
@Chralt98
Copy link
Member Author

Dropped in favor of this PR #1293 This was done, because the commit history here led to merged changes would have been needed to be reviewed again.

@Chralt98 Chralt98 closed this Mar 29, 2024
@Chralt98 Chralt98 added s:abandoned This pull request is abandoned and removed s:revision-needed The pull requests must be revised labels Mar 29, 2024
@Chralt98 Chralt98 mentioned this pull request Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:abandoned This pull request is abandoned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants