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

Fix bugs related to fidelity registration and redemption #390

Closed
wants to merge 1 commit into from

Conversation

KnowWhoami
Copy link
Collaborator

@KnowWhoami KnowWhoami commented Jan 26, 2025

This PR addresses the following issues:

Additionally, it includes some auxiliary enhancements.

Note to Reviewers

  • There are a few TODOs mentioned in the PR on which I need your feedbacks.
  • Depending on the required changes, they can be addressed in this PR or a follow-up pr .

Work left to be done:

  • Extending our tests to include these added features is still left to do.

Copy link

@mojoX911 mojoX911 left a comment

Choose a reason for hiding this comment

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

Concept ACK. Approach suggestion below.

@KnowWhoami KnowWhoami force-pushed the fidelity_fixes branch 4 times, most recently from 20fe8ec to 6b36ed5 Compare February 14, 2025 15:06
@KnowWhoami KnowWhoami requested a review from mojoX911 February 14, 2025 15:19
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 74.07407% with 35 lines in your changes missing coverage. Please review.

Project coverage is 70.71%. Comparing base (acef8db) to head (4ce52a3).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
src/maker/api.rs 36.36% 14 Missing ⚠️
src/maker/server.rs 80.55% 14 Missing ⚠️
src/wallet/fidelity.rs 83.87% 5 Missing ⚠️
src/bin/taker.rs 0.00% 1 Missing ⚠️
src/maker/rpc/server.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   70.06%   70.71%   +0.64%     
==========================================
  Files          34       34              
  Lines        4263     4258       -5     
==========================================
+ Hits         2987     3011      +24     
+ Misses       1276     1247      -29     

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

Comment on lines +140 to 159
fn manage_fidelity_bonds_and_update_dns(
maker: &Maker,
maker_addr: &str,
dns_addr: &str,
) -> Result<(), MakerError> {
maker.wallet.write()?.redeem_expired_fidelity_bonds()?;

let proof = setup_fidelity_bond(maker, maker_addr)?;

// Check for swap liquidity
check_swap_liquidity(maker)?;

let dns_metadata = DnsMetadata {
url: maker_address.clone(),
url: maker_addr.to_string(),
proof,
};

let request = DnsRequest::Post {
metadata: dns_metadata,
};
Copy link
Collaborator Author

@KnowWhoami KnowWhoami Feb 17, 2025

Choose a reason for hiding this comment

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

In every 10 min , Maker Server checks for fidelity bonds and make POST request to DNS which seems like spamming the DNS server as it makes the request even if it's fidelity proof doesn't change.
And we want to make this request so that DNS can sync with the maker's latest data and in case of DNS server failure.

So we can have a better logic :

  • In every 10 min -> call this api, but make the POST request to DNS only when maker's fidleity proof change or at an interval of 1 day.

what do you think @mojoX911 ?

@KnowWhoami
Copy link
Collaborator Author

Solving 3 issues + some other changes in a single PR makes it bloated and hard to review.
So Closing it now , and would raise dedicated PR's for each of the issues..

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