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

Chore/cleanup state and fees #325

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

0xFable
Copy link
Contributor

@0xFable 0xFable commented Apr 10, 2024

Closes #317
Closes #305
Removes some unused functions, reworks code a lil bit to accomodate changes. Made changes to fees and moved to fee.rs for custom fees too.

  • I have read Migaloo's contribution guidelines.
  • My pull request has a sound title and description (not something vague like Update index.md)
  • All existing and new tests are passing.
  • I updated/added relevant documentation.
  • The code is formatted properly cargo fmt --all --.
  • Clippy doesn't report any issues cargo clippy -- -D warnings.
  • I have regenerated the schemas if needed cargo schema.

0xFable added 2 commits April 10, 2024 01:32
…ers now not pair keys

[pool-manager] there are a few functions in the state file that are not even used. Remove if not needed #317
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 88.57%. Comparing base (f1fdf5b) to head (9911660).
Report is 1 commits behind head on release/v2_contracts.

Files Patch % Lines
packages/white-whale-std/src/fee.rs 82.97% 8 Missing ⚠️
...acts/liquidity_hub/pool-manager/src/tests/suite.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                   Coverage Diff                    @@
##           release/v2_contracts     #325      +/-   ##
========================================================
+ Coverage                 88.50%   88.57%   +0.07%     
========================================================
  Files                       252      252              
  Lines                     23602    23636      +34     
========================================================
+ Hits                      20888    20936      +48     
+ Misses                     2714     2700      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@0xFable 0xFable force-pushed the chore/cleanup-state-and-fees branch from 8a7432d to 02ac728 Compare April 10, 2024 02:16
@0xFable 0xFable force-pushed the chore/cleanup-state-and-fees branch from a563d0b to 9911660 Compare April 10, 2024 02:41
Copy link
Contributor

@kerber0x kerber0x left a comment

Choose a reason for hiding this comment

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

LGTM, well done! Why can't we remove NATIVE_TOKEN_DECIMALS already?

/// functionalities. This vector enables the flexibility to introduce new fees without altering
/// the core fee structure. Total of all fees, including custom ones, is validated to not exceed
/// 100%, ensuring a balanced and fair fee distribution.
pub extra_fees: Vec<Fee>,
Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something you came up with or did you discuss it with @Sen-Com ? I like the idea of having a vector of fees so we don't need to be changing the core structure constantly 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the 3 of us discussed it ages ago when making the spec about having the possibility of having custom fees but I thought it was already implemented. Since I was working on fees already I added this in so its just one extra thing out of the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something that comes to my mind is in future I would prefer if this was an Map or maybe slightly more complex fee such that we have a fee name included too that way we know what custom fees correspond to what.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that sounds great, good idea

pair_info.asset_decimals.clone()
}
// Remove after adding decimals to pair info
pub const NATIVE_TOKEN_DECIMALS: Map<&[u8], u8> = Map::new("allow_native_token");
Copy link
Contributor

Choose a reason for hiding this comment

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

What prevents us from removing NATIVE_TOKEN_DECIMALS right away?

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 need #306 to be done first so we have still have decimals somewhere. I could do that issue and remove NATIVE_TOKEN_DECIMALS as a part of that but until we have #306 we sort of need this.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah you are right!

@kerber0x
Copy link
Contributor

:shipit: @0xFable

@0xFable 0xFable merged commit 7440733 into release/v2_contracts Apr 11, 2024
4 of 6 checks passed
@0xFable 0xFable deleted the chore/cleanup-state-and-fees branch April 11, 2024 17: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
2 participants