Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

ApproveAndCall return values #527

Merged
merged 5 commits into from
Mar 21, 2020
Merged

ApproveAndCall return values #527

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2d71d4f
ApproveAndCall return values
Mar 16, 2020
1dc2100
ApproveAndCall return value
Mar 16, 2020
ebbfc03
Separate return value tests
Mar 16, 2020
1d862fd
remove .only() from test
Mar 17, 2020
3fcd301
Merge branch 'master' into approveAndCall-return
Shadowfiend Mar 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion implementation/contracts/system/TBTCDepositToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,11 @@ contract TBTCDepositToken is ERC721Metadata, DepositFactoryAuthority {
/// @param _spender Address of contract authorized to spend.
/// @param _tdtId The TDT they can spend.
/// @param _extraData Extra information to send to the approved contract.
function approveAndCall(address _spender, uint256 _tdtId, bytes memory _extraData) public returns (bool success) {
function approveAndCall(address _spender, uint256 _tdtId, bytes memory _extraData) public returns (bool) {
tokenRecipient spender = tokenRecipient(_spender);
approve(_spender, _tdtId);
spender.receiveApproval(msg.sender, _tdtId, address(this), _extraData);
return true;
}
}

Expand Down
3 changes: 2 additions & 1 deletion implementation/contracts/system/TBTCToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,13 @@ contract TBTCToken is ERC20Detailed, ERC20, VendingMachineAuthority {
/// @param _spender Address of contract authorized to spend.
/// @param _value The max amount they can spend.
/// @param _extraData Extra information to send to the approved contract.
function approveAndCall(address _spender, uint256 _value, bytes memory _extraData) public returns (bool success) {
function approveAndCall(address _spender, uint256 _value, bytes memory _extraData) public returns (bool) {
tokenRecipient spender = tokenRecipient(_spender);
if (approve(_spender, _value)) {
spender.receiveApproval(msg.sender, _value, address(this), _extraData);
return true;
}
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This structure seems different---do we need to introduce the same if (approve...) guard to the TDT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approve() does not return bool for ERC721

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}

Expand Down
54 changes: 54 additions & 0 deletions implementation/test/VendingMachineTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,31 @@ describe("VendingMachine", async function() {
expect(eventList[0].returnValues._digest).to.equal(sighash)
})

it("returns true on success", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of things here:

  • This block describes RedemptionScript, but as far as I can see, you're not actually returning true on success for RedemptionScript?
  • This is really a test of TBTCToken, not RedemptionScript, right?

Same note below for TBTCDepositToken vs FundingScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await testDeposit.setState(states.ACTIVE)
await tbtcDepositToken.forceMint(vendingMachine.address, tdtId)
await tbtcToken.forceMint(owner, depositValue.add(signerFee))
await feeRebateToken.forceMint(owner, tdtId)
const tbtcToBtc = vendingMachine.abi.filter(
x => x.name == "tbtcToBtc",
)[0]
const calldata = web3.eth.abi.encodeFunctionCall(tbtcToBtc, [
testDeposit.address,
"0x1111111100000000",
redeemerOutputScript,
owner,
])

const success = await tbtcToken.approveAndCall.call(
redemptionScript.address,
depositValue.add(signerFee),
calldata,
{from: owner},
)

expect(success).to.equal(true)
})

it("reverts for unknown function calls encoded in _extraData", async () => {
const unknownFunctionSignature = "0xCAFEBABE"
await tbtcToken.forceMint(owner, depositValue.add(signerFee))
Expand Down Expand Up @@ -514,6 +539,35 @@ describe("VendingMachine", async function() {
expect(await feeRebateToken.ownerOf(tdtId)).to.equal(owner)
})

it("reverts true on success", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverts -> returns ?

const unqualifiedDepositToTbtcABI = vendingMachine.abi.filter(
x => x.name == "unqualifiedDepositToTbtc",
)[0]
const calldata = web3.eth.abi.encodeFunctionCall(
unqualifiedDepositToTbtcABI,
[
testDeposit.address,
_version,
_txInputVector,
_txOutputVector,
_txLocktime,
_fundingOutputIndex,
_merkleProof,
_txIndexInBlock,
_bitcoinHeaders,
],
)

const success = await tbtcDepositToken.approveAndCall.call(
fundingScript.address,
tdtId,
calldata,
{from: owner},
)

expect(success).to.be.true
})

it("reverts for unknown function calls encoded in _extraData", async () => {
const unknownFunctionSignature = "0xCAFEBABE"

Expand Down