Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add withdrawTo functionality to support exits to other addresses #20

Merged
merged 3 commits into from
Nov 23, 2021

Conversation

QEDK
Copy link
Contributor

@QEDK QEDK commented Nov 2, 2021

No description provided.

@QEDK QEDK added the enhancement New feature or request label Nov 2, 2021
@QEDK QEDK requested a review from imyourm8 November 2, 2021 16:06
@QEDK QEDK self-assigned this Nov 2, 2021
@QEDK QEDK requested a review from jdkanani November 2, 2021 16:06
require(_isContract(_tokenTemplate), "Token template is not contract");
}

function withdraw(address childToken, uint256 id, uint256 amount, bytes memory data) public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes are these 4 functions, and the internal _withdraw functions below. Had to reset the formatting here to be similar to all other contracts.

);

// withdraw tokens
childTokenContract.burn(tokenId);
Copy link

@imyourm8 imyourm8 Nov 3, 2021

Choose a reason for hiding this comment

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

childTokenContract uses msg.sender internally? Other tokens use user's address to burn them, why does this contract uses the tunnel itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are using OpenZeppelin implementations of the ERC token contracts, in the case of ERC721 there is no such parameter, hence we don't have it either.

@jdkanani
Copy link
Contributor

jdkanani commented Nov 8, 2021

@QEDK can you please resolve the conflict?

@QEDK
Copy link
Contributor Author

QEDK commented Nov 9, 2021

@jdkanani Done! 👌

@QEDK
Copy link
Contributor Author

QEDK commented Nov 23, 2021

Merging after a conversation with @imyourm8 💯

@QEDK QEDK merged commit 99cbdc8 into main Nov 23, 2021
@QEDK QEDK deleted the withdraw-to branch November 23, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants