Zondax / filecoin-solidity

Filecoin Solidity API Library
Apache License 2.0
93 stars 43 forks source link

getDealActivation(dealID) will revert the contract if the deal is expired #363

Open ottpeter opened 1 year ago

ottpeter commented 1 year ago

getDealActivation(dealID) will revert the contract if the deal is expired.

This is the comment above the function:

/// @notice Fetches activation state for a deal.
/// @notice This will be available from when the proposal is published until an undefined period after the deal finishes (either normally or by termination).
/// @return USR_NOT_FOUND if the deal doesn't exist (yet), or EX_DEAL_EXPIRED if the deal has been removed from state.

By this, I'm assuming that I can get a return value when the deal expired, including that scenario when the start epoch past before the sealing was done.

This is the return type of the function:

struct GetDealActivationReturn {
    int64 activated;
    int64 terminated;
}

I don't know how this function would signal if the deal expired. If it is not the intended behavior that it is signaling that the deal expired, then I don't know how I should check first whether the deal is expired or not, so the smart contract call does not fail.

:link: zboto Link

ottpeter commented 1 year ago

Hi, Is there any update on this? Thanks!

rllola commented 1 year ago

Hi @ottpeter

In Filecoin if the deal is expired it will throw an error (see https://github.com/filecoin-project/builtin-actors/blob/master/actors/market/src/lib.rs#L979). Indeed the description should not indicate that it will return EX_DEAL_EXPIRED because it is actually the error that is being throw.

There is nothing that we can do in the lib for that. You could however in your code attempt to catch this error and verify it matches EX_DEAL_EXPIRED to avoid your call to fail.

ottpeter commented 1 year ago

Hi @rllola Unfortunately it is not possible to do try..catch there, because it is not a cross-contract-call. See this thread: https://filecoinproject.slack.com/archives/CRK2LKYHW/p1680872840905409

rllola commented 1 year ago

The joy of solidity...

Ok you might be able to bypass this by rewritting your own callByID function (see https://github.com/Zondax/filecoin-solidity/blob/master/contracts/v0.8/utils/Actor.sol#L102-L128).

Just remove the readRespData(data) call and parse the data yourself. The actor exit code should not be null because the deal is expired. You can probably verify that.

The other option would be to have a call in the builtin-actor that allow to ask if a deal has expired or not. (It would be the best solution in my opinion).

raulk commented 1 year ago

The problem is that the library is losing information along the way. The library needs to catch/lower the exception at the call site, parse the exit code, and propagate it as a return value.

raulk commented 1 year ago

To be more concrete, the library should not unconditionally REVERT here:

https://github.com/Zondax/filecoin-solidity/blob/461d99318dd5b0114141e128e6c4ec72b33534f5/contracts/v0.8/utils/Actor.sol#L123-L125

Instead, it should parse the exit code from the data returned by the precompile, and propagate it back.

ottpeter commented 1 year ago

See this thread on Slack: https://filecoinproject.slack.com/archives/CRK2LKYHW/p1680872840905409

rllola commented 1 year ago

@raulk We are parsing it here. https://github.com/Zondax/filecoin-solidity/blob/master/contracts/v0.8/utils/Actor.sol#L187-L189

There is still 2 solutions to this problem. The first one I already explained. The second one is to make it a cross call contract.

EDIT: I also submitted in the FIP forum a proposition to add status field which would properly inform about the activation status of the deal. No new call just instead of throwing an error return the status.

raulk commented 1 year ago

@rllola we might be talking past each other. The correct solution is for this library to not force a revert, but rather relay the exit code to the user via a return parameter, and let the user handle how they wish. Generally speaking, exit codes convey all kinds of important information that this library is currently dismissing. The concrete example case here is just one example of many where this unconditional low-level revert will be problematic and insufficient.

raulk commented 1 year ago

Can we please build a PoC?

raulk commented 1 year ago

@raulk We are parsing it here. https://github.com/Zondax/filecoin-solidity/blob/master/contracts/v0.8/utils/Actor.sol#L187-L189

Good, but the library still reverts unconditionally, making the exit code useless for the contract that's actually using the library.