Skip to content

feat: use subsidies contract to proxy register blob call #2151

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zihehuang
Copy link
Contributor

@zihehuang zihehuang commented May 30, 2025

Description

Describe the changes or additions included in this PR.

Test plan

How did you test the new or updated feature?


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that
a user might notice and any actions they must take to implement updates. (Add release notes after the colon for each item)

  • Storage node:
  • Aggregator:
  • Publisher:
  • CLI: use subsidies contract to proxy register blob call

@zihehuang zihehuang changed the title feat: proxy subsidies contract register blob call feat: use subsidies contract to proxy register blob call May 30, 2025
Copy link
Contributor

@karlwuest karlwuest left a comment

Choose a reason for hiding this comment

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

This looks good! I added a few comments and it would be great to have some tests to make sure that everything works as expected (including for 100% subsidies and empty WAL coins).

Comment on lines +1490 to +1491
full_price * (MAX_SUBSIDY_RATE - subsidies_object.buyer_subsidy_rate as u64)
/ MAX_SUBSIDY_RATE
Copy link
Contributor

Choose a reason for hiding this comment

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

Important: I think this calculation is not quite correct, because it rounds towards zero, so if the numerator is not divisible by MAX_SUBSIDY_RATE, one FROST may be missing in the calculated amount. Instead, we should always round up.

@@ -45,6 +45,9 @@ use crate::{
wallet::Wallet,
};

/// Max subsidy rate is 100%.
pub const MAX_SUBSIDY_RATE: u64 = 10_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/Remark: Let's call this MAX_BUYER_SUBSIDY_RATE instead of MAX_SUBSIDY_RATE just for clarity and future proofing. With the planned changes to the subsidies, we may want to have system subsidies above 100% so that will probably also change in the contracts.

},
)
.await
if blob_metadata_list.is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: This now duplicates a lot of code with reserve_and_register_blobs_without_subsidies. Can we still keep an "inner" function, but instead of passing two closures for reserve and register, we just pass a flag with_subsidies (and then make a case distinction at the relevant points in the function)?

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