fix: isCancelable in getStream
#498
Conversation
PaulRBerg
force-pushed
the
fix/get-stream-is-cancelable
branch
2 times, most recently
from
0d78723 to
4ccfea8
Compare
test: rename "streamId" to "defaultStreamId" test: update "getStream" tests
PaulRBerg
force-pushed
the
fix/get-stream-is-cancelable
branch
from
4ccfea8 to
0034359
Compare
PaulRBerg
force-pushed
the
fix/get-stream-is-cancelable
branch
from
0034359 to
c0df5e7
Compare
There was a problem hiding this comment.
I think this solution is not ideal because it kinda "lies" about the state of the contract(and minor thing would be the gas implications), but it works. In my opinion renaming the isCancelable storage variable to something like cancebilityBeforeSettledwould have been better.
If you added the internal _statusOf function, we can make it virtual to add it SablierV2Lockup contract and use it in the renounce function(notNull must be added).
notNull modifier implicitly called in isCold function:
| @@ -98,7 +98,7 @@ interface ISablierV2Lockup is | |||
| function getWithdrawnAmount(uint256 streamId) external view returns (uint128 withdrawnAmount); | |||
|
|
|||
| /// @notice Retrieves a flag indicating whether the stream can be canceled. When the stream is cold, this | |||
| /// flag is always `false` regardless of the value of the struct field `isCancelable`. | |||
| /// flag is always `false`. | |||
There was a problem hiding this comment.
Let's say something like: returns it as it would be false.
There was a problem hiding this comment.
Nope. See my comment below. It doesn't matter what is in contract storage; only what end users see.
It's not "as if it would be false" - from the perspective of end users, it is false.
test/fuzz/lockup-dynamic/create-with-milestones/createWithMilestones.t.sol
Show resolved
Hide resolved
| @@ -2,4 +2,7 @@ getStream.t.sol | |||
| ├── when the id references a null stream | |||
| │ └── it should revert | |||
| └── when the id does not reference a null stream | |||
| └── it should return the stream | |||
| ├── when the stream is settled | |||
| │ └── it should adjust and return the stream | |||
There was a problem hiding this comment.
Let's say "adjust the is cancelable flag"
There was a problem hiding this comment.
Fine, but let's say isCancelable (with backticks) to make it clear what we're referring to.
There was a problem hiding this comment.
I think this solution is not ideal because it kinda "lies" about the state of the contract(and minor thing would be the gas implications), but it works. In my opinion renaming the isCancelable storage variable to something like cancebilityBeforeSettledwould have been better.
If you added the internal _statusOf function, we can make it virtual to add it SablierV2Lockup contract and use it in the renounce function(notNull must be added).
notNull modifier implicitly called in isCold function:
We are not lying about anything - could you please look at this problem from the end users' perspective? The state of the contract != storage (in a strict sense). The state of the contract is what is made available in the public API. As I have said in private, it doesn't matter what happens internally in a piece of software as long as the software works as expected for end users.
Good idea.
Good catch |
fix: mark "_statusOf" as "internal" perf: mark "statusOf" as "external" test: improve writing in "getStream" state trees
I am, but I suppose it is harder for me, as an engineer who understands what's underneath.
IMO smart contracts are a special type of software, so this is why I consciously prefer to be paranoid about things. At the moment I can't provide a malicious scenario that could arise from this, so we can leave it as it is. |
|
Smart contracts are special, but this is not a prerogative for not solving problems when they arise (my PR solves a problem, although it might do so in a clumsy way, as you suggest). Anyway. If we can leave it as is, can you approve the PR? |
Not sure which version of the contract you have reviewed, but that commit ( |
Addresses #497.
Note: this should have been done in #467.