Skip to content

Conversation

@DarkLord017
Copy link
Contributor

Solves #350

I added validatePayerFunds in piecesAdded instead of updating it in nextProvingPeriod.

When a piece is scheduled for deletion, we can’t remove it directly there because the payment needs to be updated. So I didn’t remove it from nextProvingPeriod, but added a check to see if pieces were actually removed.

The call will revert if the payment rates are not updated and nothing was deleted, so this check prevents that.

@FilOzzy FilOzzy added this to FS Dec 6, 2025
@github-project-automation github-project-automation bot moved this to 📌 Triage in FS Dec 6, 2025
@rjan90 rjan90 moved this from 📌 Triage to 🔎 Awaiting review in FS Dec 8, 2025
@rvagg
Copy link
Collaborator

rvagg commented Dec 8, 2025

@DarkLord017 we want to avoid adding new storage slots wherever possible, and if we do add them, they have to go last in the list so the contract is upgradable. But I'm pretty sure we have no need for state here at all. You're welcome to check my logic here though and correct me if you think I'm wrong:

  • piecesAdded in FWSS gets called after addPieces in PDPVerifier which updates the leaf count there before we see it, there's no delay on adding pieces like there is for deletions. So IPDPVerifier(pdpVerifierAddress).getDataSetLeafCount(dataSetId) is always going to get us the correct leaf count when we want to set the new rate in piecesAdded.
  • When nextProvingPeriod in FWSS gets called, any scheduled deletions will have already happened in PDPVerifier, so calling IPDPVerifier(pdpVerifierAddress).getDataSetLeafCount(dataSetId) in nextProvingPeriod should always get us the correct leaf count - regardless of what additions or deletions have happened prior to this call.
  • In FilecoinPayV1's modifyRailPayment, when oldRate == newRate it's a noop, so it's safe to call and we only waste a little gas to make that cross-contract call.

I think in #343 we are going to need to add state to track deletions, and when that's done we could be more conditional in nextProvingPeriod and only update the rates when there are deletions and save a little bit of gas. But for now I don't think we're any worse off in gas terms than we are today.

@DarkLord017
Copy link
Contributor Author

@DarkLord017 we want to avoid adding new storage slots wherever possible, and if we do add them, they have to go last in the list so the contract is upgradable. But I'm pretty sure we have no need for state here at all. You're welcome to check my logic here though and correct me if you think I'm wrong:

  • piecesAdded in FWSS gets called after addPieces in PDPVerifier which updates the leaf count there before we see it, there's no delay on adding pieces like there is for deletions. So IPDPVerifier(pdpVerifierAddress).getDataSetLeafCount(dataSetId) is always going to get us the correct leaf count when we want to set the new rate in piecesAdded.
  • When nextProvingPeriod in FWSS gets called, any scheduled deletions will have already happened in PDPVerifier, so calling IPDPVerifier(pdpVerifierAddress).getDataSetLeafCount(dataSetId) in nextProvingPeriod should always get us the correct leaf count - regardless of what additions or deletions have happened prior to this call.
  • In FilecoinPayV1's modifyRailPayment, when oldRate == newRate it's a noop, so it's safe to call and we only waste a little gas to make that cross-contract call.

I think in #343 we are going to need to add state to track deletions, and when that's done we could be more conditional in nextProvingPeriod and only update the rates when there are deletions and save a little bit of gas. But for now I don't think we're any worse off in gas terms than we are today.

Yeah, that makes sense. I was thinking the same — my intent was just to skip the update when the leaf count hasn’t changed to save a bit of gas, so I added that check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🔎 Awaiting review

Development

Successfully merging this pull request may close these issues.

2 participants