Skip to content

feat!: Rails library#476

Merged
wjmelements merged 14 commits into
mainfrom
rails-lib
May 14, 2026
Merged

feat!: Rails library#476
wjmelements merged 14 commits into
mainfrom
rails-lib

Conversation

@wjmelements
Copy link
Copy Markdown
Contributor

@wjmelements wjmelements commented May 13, 2026

Reviewer @rvagg
Toward #469
I am cutting a PR here in order to help keep the code review digestible.
I think reviewing the changes commit-by-commit might also be helpful.
This reduces contract size from 23939 to 19857

Changes

  • move pricing constants and functions to lib/PriceListUSDFC.sol
  • move rails logic to lib/Rails.sol
  • move calculateRatePerEpoch to the View contract, update gen to allow pure methods in the view contract.
  • remove methods updateServiceCommission and updatePricing; these parameters will now be managed by the upgrade path
  • update deployment scripts to deploy the rails lib

@wjmelements wjmelements linked an issue May 13, 2026 that may be closed by this pull request
2 tasks
@wjmelements wjmelements requested a review from rvagg May 13, 2026 21:21
@FilOzzy FilOzzy added this to FOC May 13, 2026
@github-project-automation github-project-automation Bot moved this to 📌 Triage in FOC May 13, 2026
uint256 constant EPOCHS_PER_MONTH = 2880 * 30;

// USDFC has 18 decimals, so $1 = 10**18 (a.k.a. ether)
uint256 constant SYBIL_FEE = 0.1 ether;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this was originally a const then we exposed it on PDPVerifier and used IPDPVerifier(pdpVerifierAddress).USDFC_SYBIL_FEE() and now we're back to a const here, is this intentional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think a const in the PriceList is the right way to go. We would run into trouble if the pdp verifier disagreed with this but maybe we can remove the pdp verifier's sybil check once we adopt the cleanup rebate system.

Comment thread service_contracts/src/lib/Rails.sol
Comment thread service_contracts/abi/FilecoinWarmStorageService.abi.json
@rvagg
Copy link
Copy Markdown
Collaborator

rvagg commented May 14, 2026

SPEC.md needs updating, it still has "Only the contract owner can update pricing by calling updatePricing(newStoragePrice, newMinimumRate) ..."; that'll need fixing.

@rvagg
Copy link
Copy Markdown
Collaborator

rvagg commented May 14, 2026

Oh, and README.md also says "The service uses static global pricing set by the contract owner (default: 2.5 USDFC per TiB/month)". I guess it's still mostly true, just not a setter, we could leave this, your call.

Copy link
Copy Markdown
Collaborator

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

I normally hate per-commit reviewing but this is really nicely separated out, so it was actually a good experience (and I hadn't caught up on GitHub making it a bit easier too); very nice work.
Just a few things to take care of but this is a preemptive approval assuming you'll look at & take care of these things:

  • the sybil fee fetcher question
  • duplicate DEFAULT_LOCKUP_PERIOD import
  • would like somewhere mentioning the calculateRatePerEpoch removal, this is technically a breaking change, we could signal it with ! but calling it out in a commit message would be very helpful too, the commit refactor: Rails.updateStorageRates could become refactor!: Rails.updateStorageRates and in the body mention that calculateRatePerEpoch was moved off the proxy.
  • SPEC.md needs updating to talk about how the pricing gets changed now

@github-project-automation github-project-automation Bot moved this from 📌 Triage to ✔️ Approved by reviewer in FOC May 14, 2026
@rvagg
Copy link
Copy Markdown
Collaborator

rvagg commented May 14, 2026

I think we could probably remove calculateRatePerEpoch entirely when we're done here, may not be the worst thing to remove it entirely even from here and just document the breakage

@BigLep BigLep moved this from ✔️ Approved by reviewer to ⌨️ In Progress in FOC May 14, 2026
@wjmelements
Copy link
Copy Markdown
Contributor Author

I think we could probably remove calculateRatePerEpoch entirely when we're done here, may not be the worst thing to remove it entirely even from here and just document the breakage

I'm inclined to keep it for now because it's used to test the pure function. It can be calculated from the information in getServicePrice, and isn't that useful on-chain, so I agree it should be removed as part of #469

@wjmelements wjmelements merged commit 32b9660 into main May 14, 2026
6 checks passed
@wjmelements wjmelements deleted the rails-lib branch May 14, 2026 22:16
@github-project-automation github-project-automation Bot moved this from ⌨️ In Progress to 🎉 Done in FOC May 14, 2026
@wjmelements wjmelements changed the title feat: Rails library feat!: Rails library May 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🎉 Done

Development

Successfully merging this pull request may close these issues.

4 participants