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

Router settle with balance #235

Merged
merged 5 commits into from
Aug 2, 2024
Merged

Router settle with balance #235

merged 5 commits into from
Aug 2, 2024

Conversation

hensha256
Copy link
Contributor

Allow settle to be funds from the router

@@ -27,7 +27,11 @@ contract V4RouterImplementation is V4Router, ReentrancyLock {
}

function _pay(Currency token, address payer, uint256 amount) internal override {
ERC20(Currency.unwrap(token)).safeTransferFrom(payer, address(poolManager), amount);
if (payer == address(this)) {
ERC20(Currency.unwrap(token)).safeTransfer(address(poolManager), amount);
Copy link
Member

Choose a reason for hiding this comment

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

why not use currency.transfer from currency library?

Copy link
Contributor Author

@hensha256 hensha256 Aug 1, 2024

Choose a reason for hiding this comment

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

🤷‍♀️ its a test so didnt think about it tbh, will do it now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! i also moved this contract to /mocks and deleted /implementations

@hensha256 hensha256 requested a review from snreynolds August 1, 2024 09:36
snreynolds
snreynolds previously approved these changes Aug 1, 2024
@@ -27,7 +28,11 @@ contract V4RouterImplementation is V4Router, ReentrancyLock {
}

function _pay(Currency token, address payer, uint256 amount) internal override {
ERC20(Currency.unwrap(token)).safeTransferFrom(payer, address(poolManager), amount);
if (payer == address(this)) {
token.transfer(address(poolManager), amount);
Copy link
Member

Choose a reason for hiding this comment

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

so I think we both have the same problem in pay where by this point (if its been called from DeltaResolver) it definitely wont be NATIVE currency, but we still check if its native inside currency. I wonder if we should write a currency.transferERC20() without that native check.. anyways food for thought.

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 contract is actually just a mock for tests - in the universal router (where i implement _pay) it doesnt check if its ETH. But its a good thought for posm

_settle(currency, _msgSender(), amount);
_settle(currency, _msgSender(), _getFullSettleAmount(currency));
} else if (action == Actions.SETTLE_WITH_BALANCE) {
Currency currency = params.decodeCurrency();
Copy link
Member

Choose a reason for hiding this comment

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

i like how you do this here, Im going to change posm to do this as well

@hensha256 hensha256 merged commit a421a06 into main Aug 2, 2024
3 checks passed
@hensha256 hensha256 deleted the router-settle-with-balance branch August 2, 2024 12:50
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.

2 participants