feat(piecesAdded):validatePayerFunds #352
Conversation
| assertEq(dataSetId, 1, "Dataset should be created with above-minimum funds"); | ||
| } | ||
|
|
||
| // function testInsufficientFunds_AddPieces() public { |
There was a problem hiding this comment.
what's going on here? why is it commented out?
There was a problem hiding this comment.
like i made this PR for initial review like if the implementation sg than i can work on tests
There was a problem hiding this comment.
ok, feel free to proceed, I think you have a good handle on the task at hand, just look for opportunities to minimise duplication as I mentioned above
|
Also solves #350 |
| dataSetPieceMetadataKeys[dataSetId][pieceId].push(key); | ||
| } | ||
| uint256 leaves = IPDPVerifier(pdpVerifierAddress).getDataSetLeafCount(dataSetId); | ||
| updatePaymentRates(dataSetId, leaves); |
There was a problem hiding this comment.
Let's not do this in here; the payment rate change from #350 is a much larger behavioural change than just validting funds. We ought not to mix the two up since they are different things and should be both considered and merged separately (perhaps we want to revert one but not the other, or stagger the order in which we deploy these, or maybe the behavioural change ends up creating new bugs and the mixing of these two things makes it harder to diagnose).
There was a problem hiding this comment.
if you just want to split this up into two separate PRs then that's fine by me; you can stack them if you like so the second one has both changes so that when we merge this one for validatePayerFunds it'll disappear from (or be rebased out of) the second one
There was a problem hiding this comment.
OK will split the prs
| /// @notice Validate payer/operator approvals and funds for adding pieces | ||
| /// @dev Computes the new storage rate and corresponding 30-day lockup after adding `pieceData` | ||
| /// and validates the payer has sufficient available funds and operator allowances. | ||
| function validatePayerOperatorApprovalAndFundsForPieces( |
There was a problem hiding this comment.
How about instead of making a new function here, you just extend validatePayerOperatorApprovalAndFunds and allow it to take a leaf count - zero in the case of createDataSet and the actual count for the piecesAdded function - then we don't have so much code duplication.
Perhaps there's some nuance I'm missing between that case and this one but it seems to me that they are basically the same, just one has leaves and the other deals in minimums. In fact, there's some logic in validatePayerOperatorApprovalAndFunds that does minimum rate calculation that could be replaced with a call to _calculateStorageRate; so it may be that there's no need for special casing inside the function, it just needs to take a leafCount which may or may not be zero.
|
Aside from the changes above, we should take a critical look at what happens when we do #350 because maybe all of this nice work is unnecessary and would be a waste of gas. Since we're doing a rail modification in the call anyway we're going to be getting these checks by Filecoin Pay. My lingering question is over the UX of it—do we get what we want in terms of error information from Filecoin Pay because I think the only thing this might add after #350 is nicer sugar consistent with the CreateDataSet checks. |
Please see #365 |
|
Also ig this pr will resolve the issue #351 completely |
2 participants
fixes #351
validatePayerOperatorApprovalAndFundsForPiecestoFilecoinWarmStorageServicethat checks payer/operator approvals, available funds, and allowance limits before allowing new pieces to be added.IPDPVerifierinterface to enable querying the current leaf count from the PDP verifier, which is used in the new validation logic.does the implementation loog good should i move to adding tests ?