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

TransformERC20: Fix selling entire ETH balance #46

Merged

Conversation

dorothy-zbornak
Copy link
Contributor

Description

In the TransformERC20Feature, when inputTokenAmount == uint256(-1), we interpret this as trying to sell the taker's entire available balance. But we did not check if the input token was ETH. This causes reverts because 0xeeeee... is not a token. Now we just set inputTokenAmount = msg.value when ETH is being sold and inputTokenAmount == uint256(-1).

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

Copy link
Contributor

@smarx smarx left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@hysz hysz left a comment

Choose a reason for hiding this comment

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

LGTM

@dorothy-zbornak dorothy-zbornak force-pushed the fix/ep-transform-erc20/eth-sell-all-revert branch from a9a6494 to 1432bec Compare November 24, 2020 20:23
…elling taker's entire balance when input token is ETH
@dorothy-zbornak dorothy-zbornak force-pushed the fix/ep-transform-erc20/eth-sell-all-revert branch from 1432bec to 92a1fba Compare November 24, 2020 22:35
@dorothy-zbornak dorothy-zbornak merged commit ca20df4 into development Nov 24, 2020
@dorothy-zbornak dorothy-zbornak deleted the fix/ep-transform-erc20/eth-sell-all-revert branch November 24, 2020 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants