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

Pay with balance, and use delta as amount in #249

Merged
merged 14 commits into from
Aug 2, 2024
Merged

Conversation

hensha256
Copy link
Contributor

No description provided.

test/mocks/MockV4Router.sol Show resolved Hide resolved
src/V4Router.sol Outdated Show resolved Hide resolved
src/V4Router.sol Outdated
@@ -63,12 +65,11 @@ abstract contract V4Router is IV4Router, BaseActionsRouter, DeltaResolver {
}

function _swapExactInputSingle(IV4Router.ExactInputSingleParams memory params) private {
uint128 amountIn = (params.amountIn == OPEN_DELTA)
Copy link
Member

Choose a reason for hiding this comment

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

can this be moved to a library? this is for sure shared logic / will be shared logic

src/V4Router.sol Outdated
Currency currencyIn = params.currencyIn;
uint128 amountIn =
Copy link
Member

Choose a reason for hiding this comment

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

lib could be used here too
so we dont have to repeat amount == OPEN_DELTA ? __ : ___

Copy link
Member

@snreynolds snreynolds left a comment

Choose a reason for hiding this comment

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

i like, also just my comments about shared lib from before

src/V4Router.sol Outdated Show resolved Hide resolved
@@ -11,6 +11,10 @@ import {Actions} from "../libraries/Actions.sol";
abstract contract BaseActionsRouter is SafeCallback {
using CalldataDecoder for bytes;

/// @notice used to signal that the recipient of an action should be the _msgSender of address(this)
address internal constant MSG_SENDER = address(1);
Copy link
Member

Choose a reason for hiding this comment

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

can we just have a shared constants file with all of these?

msg sender, address this, contract balance, open delta?

assertEq(mappedRecipient, address(0xdeadbeef));
} else if (recipient == Actions.ADDRESS_THIS) {
} else if (recipient == router.ADDRESS_THIS_()) {
Copy link
Member

Choose a reason for hiding this comment

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

i would prefer this is Constants.ADDRESS_THIS/ Constants.MSG_SENDER

esp for msg sender its confusing bc you can actually query the true msg sender on the router/posm contract with router.msgSender()

}

/// @notice Calculates the amount for a swap action
function _mapSwapAmount(uint128 amount, Currency currency) internal view returns (uint128) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Since I believe we will use this function we can use a more neutral name like mapInputAmount

@snreynolds snreynolds self-requested a review August 2, 2024 21:47
@snreynolds snreynolds merged commit 30863cc into main Aug 2, 2024
3 checks passed
@snreynolds snreynolds deleted the amount-mapping branch August 2, 2024 21:48
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