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

Add looser block deadline in order rewards query #96

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

harisang
Copy link
Contributor

@harisang harisang commented Apr 5, 2024

I am tempted to say that there is a bug in the order rewards query. Specifically, it seems that the following case might happen:

  • we look at all trades executed between a start_block and an end_block in the trade_hashes table.
  • we then pull additional information about these trades, in order to compute protocol fees etc, and these computations start with the order_surplus table. But in that table, which contains information from the settlement_scores table, we ask that the deadline of the settlements we look into is end_block. I think this can cause some information to be lost, because we do a left outer join of the trade_hashes table with everything else, so every trade executed between start_block and end_block will be included in our result, while not all information about these trades will have been recovered, due to the restricted block_deadline. This is because a trade can execute at block x, while its deadline might be up to x+9, i believe.

To address this, we propose to add some very generous buffer in the block_deadline considered, so as to ensure all relevant info about the trades are pulled together and joined in the final table.

Note: this was first noticed when i was trying to fix the integration test for PR #94 , and because of that, I opened issue #95. This PR addresses this issue.

As a final comment, an alternative would be to actually not do the above and try to only use block_deadline, similar to the batch rewards query.

@harisang harisang requested a review from fhenneke April 5, 2024 13:58
Comment on lines 74 to 75
ss.block_deadline >= {{start_block}}
AND ss.block_deadline <= {{end_block}}
AND ss.block_deadline <= {{end_block}} + 100
Copy link
Contributor

@fhenneke fhenneke Apr 9, 2024

Choose a reason for hiding this comment

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

The mismatch of block_deadline and block_number is unfortunate.

(edit: looks good after thinking about it a bit more) If we want to go with that fix, would we not have to decrease the staring value? Something like

        ss.block_deadline >= {{start_block}} - 100
        AND ss.block_deadline <= {{end_block}}

We always have block_deadline > block_number and (for valid settlements) block_number > block_deadline - 100.

Alternatively, the query for trade_hashes could contain the block deadline via

        JOIN settlement_scores ss ON s.auction_id = ss.auction_id
    WHERE
        ss.block_deadline > {{start_block}}
        AND ss.block_deadline <= {{end_block}}

similarly to how it is done in batch rewards. Then the query itself would be a bit nicer, but potentially there is a mismatch with double counting after a resync.

Copy link
Contributor

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

Looks good.
A comment should be added now or later. There is a slight cleaner design, I think, but we can switch to that later.

@harisang
Copy link
Contributor Author

harisang commented Apr 9, 2024

ok let's issue a release now with this, and we can revisit it soon as we will probably need some minor changes regarding integrator fees that should be implemented in the next few days.

@harisang harisang merged commit 97e9436 into main Apr 9, 2024
6 checks passed
@harisang harisang deleted the fix_issue_with_order_rewards_block_deadline branch April 9, 2024 08:55
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.

2 participants