[Fil 1232] updated validations for transfer method in Client SC#44
Conversation
RafalMagrys
force-pushed
the
FIL-1232-sla-client-sc-correctly-verify-beneficiary-expiration-when-creating-allocations
branch
from
ef5d82b to
bf8ad5a
Compare
RafalMagrys
requested review from
Filip-L,
Szooot and
kacperzuk-neti
and removed request for
Filip-L and
kacperzuk-neti
src/Client.sol
Outdated
| ); | ||
| int64 beneficiaryExpiration = CommonTypes.ChainEpoch.unwrap(beneficiary.active.term.expiration); | ||
|
|
||
| if (longestClaimExtension.termMax > beneficiaryExpiration + 250 weeks) { |
There was a problem hiding this comment.
termMax is relative to when the claim starts. so we should look at expiration + termMax instead of just termMax.
There was a problem hiding this comment.
Why is 250 weeks added here? Shouldn't it be 180 days?
There was a problem hiding this comment.
but the claim doesn't have expiration decoded from params. it only has provider, claimId, termMax
RafalMagrys
force-pushed
the
FIL-1232-sla-client-sc-correctly-verify-beneficiary-expiration-when-creating-allocations
branch
from
3aa7889 to
ac7370c
Compare
RafalMagrys
force-pushed
the
FIL-1232-sla-client-sc-correctly-verify-beneficiary-expiration-when-creating-allocations
branch
from
ac7370c to
1b780d2
Compare
| uint256 operatorDataLength; | ||
| uint256 allocationRequestsLength; | ||
| uint256 claimExtensionRequestsLength; | ||
| uint256 resultLength; |
There was a problem hiding this comment.
Why did you change the names of these variables? The previous ones were more readable. Was this due to Solhint or the compiler?
There was a problem hiding this comment.
Minimizing number of local variables to fit in the 15 slot stack depth limit of EVM.
| struct ProviderAllocation { | ||
| CommonTypes.FilActorId provider; | ||
| uint64 size; | ||
| int64 allocationTime; |
There was a problem hiding this comment.
Do we really need to store allocationTime for every allocation? I think this might be unnecessarily gas-consuming. Maybe we could move it to a separate struct or use a different approach.
There was a problem hiding this comment.
I moved it to a new struct
| function _verifyBeneficiaryExpiration(ProviderAllocation memory longestAllocation) internal view { | ||
| if (longestAllocation.allocationTime != 0) { | ||
| MinerTypes.GetBeneficiaryReturn memory beneficiary = | ||
| MinerUtils.getBeneficiaryWithChecks(longestAllocation.provider, beneficiaryFactory, true, true, true); |
There was a problem hiding this comment.
getBeneficiaryWithChecks is used in _verifyAndRegisterAllocations, which might be the best place to validate the expiration. Within a single transfer, there can be more than one provider, each with a different beneficiary expiration time, but the check is only for one provider.
There was a problem hiding this comment.
I can surely move it here, but why should I check everyone when in the task it is written to check only Miner’s beneficiary related to this longest allocation?
| MinerUtils.getBeneficiaryWithChecks(longestAllocation.provider, beneficiaryFactory, true, true, true); | ||
| int64 beneficiaryExpiration = CommonTypes.ChainEpoch.unwrap(beneficiary.active.term.expiration); | ||
|
|
||
| if (longestAllocation.allocationTime > beneficiaryExpiration + 180 days) { |
There was a problem hiding this comment.
The beneficiary must expire no earlier than 180 days after the allocation ends.
longestAllocation.allocationTime + 180 days > beneficiaryExpiration
3 participants
No description provided.