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

Store price improvement fee #2470

Merged
merged 15 commits into from
Mar 8, 2024
Merged

Conversation

squadgazzz
Copy link
Contributor

@squadgazzz squadgazzz commented Mar 5, 2024

Description

Fixes #2478

Changes

  • max_volume_factor column is renamed to surplus_max_volume_factor in order to align with the naming convention
  • Postgres roundtrip tests
  • Readme

Copy link

github-actions bot commented Mar 5, 2024

Reminder: Please update the DB Readme.


Caused by:

@squadgazzz squadgazzz marked this pull request as ready for review March 5, 2024 12:21
@squadgazzz squadgazzz requested a review from a team as a code owner March 5, 2024 12:21
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Code looks good (except that this Row struct is becoming huge and not local to this component)

Closes task 6 of the #2287

I think it's task 5 as of right now (but it might change). From a project management perspective I think it would be better to create issues for each of the items in the overarching task. This would also give us a better overview of how much work is left and how our velocity is going in the Kanban board.

anyhow::anyhow,
bigdecimal::BigDecimal,
number::conversions::{big_decimal_to_u256, u256_to_big_decimal},
};

#[derive(Debug, Clone, PartialEq, sqlx::FromRow)]
pub struct FeePolicy {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this struct private and have insert_batch take the domain representation of fee_policy? This way we can keep this pretty horrendous type local to this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored, but not sure if it's possible to make it private since it is used in the outer mod.

database/sql/V063__price_improvement_policy_fee.sql Outdated Show resolved Hide resolved
database/README.md Outdated Show resolved Hide resolved
Comment on lines 10 to 11
ADD COLUMN price_improvement_factor double precision,
ADD COLUMN price_improvement_max_volume_factor double precision,
Copy link
Contributor

Choose a reason for hiding this comment

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

This data model still seems super sketchy to me. Unfortunately I don't really know how to make it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could move to JSONB to avoid column growth.

crates/autopilot/src/infra/persistence/dto/fee_policy.rs Outdated Show resolved Hide resolved
impl From<FeePolicy> for domain::fee::Policy {
fn from(row: FeePolicy) -> domain::fee::Policy {
match row.kind {
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this dead code now? Can we remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was never used. Removed.

@squadgazzz squadgazzz force-pushed the 2287/price-improvement-persistence branch 2 times, most recently from 50c8bd0 to 506e9be Compare March 7, 2024 18:22
@squadgazzz squadgazzz enabled auto-merge (squash) March 8, 2024 13:00
@squadgazzz squadgazzz disabled auto-merge March 8, 2024 13:00
@squadgazzz squadgazzz merged commit 55d7a6d into main Mar 8, 2024
9 checks passed
@squadgazzz squadgazzz deleted the 2287/price-improvement-persistence branch March 8, 2024 13:26
@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persist Price Improvement protocol fee
3 participants