ApproveAndCall return values #527
Conversation
Success was not defined, so the function was always returning false.
Enforce approveAndCall returns true on successful flow
| ) | ||
|
|
||
| expect(success).to.equal(true) | ||
|
|
There was a problem hiding this comment.
If we add this, do we still need the approveAndCall below?
There was a problem hiding this comment.
Yeah, this one is .call so we don't get any state change
There was a problem hiding this comment.
So that we can observe the success, I guess…
These feel like separate tests. Any reason we've glommed them on to the existing ones rather than firing up a separate test that handles approveAndCall specifically?
| tokenRecipient spender = tokenRecipient(_spender); | ||
| if (approve(_spender, _value)) { | ||
| spender.receiveApproval(msg.sender, _value, address(this), _extraData); | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
This structure seems different---do we need to introduce the same if (approve...) guard to the TDT?
There was a problem hiding this comment.
approve() does not return bool for ERC721
| @@ -27,7 +27,8 @@ const _expectedUTXOoutpoint = | |||
| "0x5f40bccf997d221cd0e9cb6564643f9808a89a5e1c65ea5e6530c0b51c18487c00000000" | |||
| const _outValueBytes = "0x2040351d00000000" | |||
|
|
|||
| describe("VendingMachine", async function() { | |||
| // eslint-disable-next-line no-only-tests/no-only-tests | |||
| @@ -443,6 +444,31 @@ describe("VendingMachine", async function() { | |||
| expect(eventList[0].returnValues._digest).to.equal(sighash) | |||
| }) | |||
|
|
|||
| it("returns true on success", async () => { | |||
There was a problem hiding this comment.
Couple of things here:
- This block describes
RedemptionScript, but as far as I can see, you're not actually returning true on success forRedemptionScript? - This is really a test of
TBTCToken, notRedemptionScript, right?
Same note below for TBTCDepositToken vs FundingScript.
There was a problem hiding this comment.
This is really a test of TBTCToken
Yes In a way. In order to test approveAndCall specifically, we would still have to go through receiveApproval, and this is implemented in our funding/redemption scripts. I can see how it's not ideal.
The tests themselves are testing approveAndCall though.
returns true on success -> approveAndCall returns true on success?
There was a problem hiding this comment.
The tests themselves are testing approveAndCall though.
I believe we should add isolated tests to contract-related test suites like TBTCDepositTokenTest.js and TBTCTokenTest.js to cover there just approveAndCall functions calls with some simple stubs.
There was a problem hiding this comment.
Sorry, I should have made my point clearer: this test is entirely a test of TBTCToken, so it feels like it should live in TBTCTokenTest. Similarly, the TDT one should live in TBTCDepositTokenTest. It feels fine to wrap these up into one TokenTest file or similar, but putting them in unrelated tests is not ideal.
In the interest of keeping things moving I'm going to merge this as-is, since it implements the underlying functionality we're looking for---but let's strive to make the changes quickly unless there was a good reason to have it in the current structure.
Run all tests, not just VendingMachine...
| @@ -514,6 +539,35 @@ describe("VendingMachine", async function() { | |||
| expect(await feeRebateToken.ownerOf(tdtId)).to.equal(owner) | |||
| }) | |||
|
|
|||
| it("reverts true on success", async () => { | |||
| @@ -443,6 +444,31 @@ describe("VendingMachine", async function() { | |||
| expect(eventList[0].returnValues._digest).to.equal(sighash) | |||
| }) | |||
|
|
|||
| it("returns true on success", async () => { | |||
There was a problem hiding this comment.
The tests themselves are testing approveAndCall though.
I believe we should add isolated tests to contract-related test suites like TBTCDepositTokenTest.js and TBTCTokenTest.js to cover there just approveAndCall functions calls with some simple stubs.
closes: #505
approveAndCallalways returns false because the return value bool success is never set.