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

[API-3529] Implement GoFastHandler contract #15

Merged
merged 10 commits into from
Nov 13, 2024
Merged

Conversation

thal0x
Copy link
Member

@thal0x thal0x commented Nov 4, 2024

Implements the GoFastHandler contract which supports swapping and then submitting a fast transfer order

@thal0x thal0x self-assigned this Nov 4, 2024
@thal0x thal0x changed the title implement GoFastHandler contract [API-3529] Implement GoFastHandler contract Nov 4, 2024
Copy link
Collaborator

@dhfang dhfang left a comment

Choose a reason for hiding this comment

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

What do you think of having the GoFastHandler in a separate top level contract?

AxelarHandler/src/GoFastHandler.sol Outdated Show resolved Hide resolved
AxelarHandler/src/GoFastHandler.sol Show resolved Hide resolved
AxelarHandler/src/GoFastHandler.sol Show resolved Hide resolved
AxelarHandler/src/GoFastHandler.sol Show resolved Hide resolved
AxelarHandler/src/GoFastHandler.sol Outdated Show resolved Hide resolved
@thal0x
Copy link
Member Author

thal0x commented Nov 5, 2024

What do you think of having the GoFastHandler in a separate top level contract?

The reason I didn't do this is because over the weekend I'm planning on removing all of that nesting and putting all contracts in a single project

@thal0x thal0x requested a review from dhfang November 8, 2024 18:24
}

function _swap(address tokenIn, uint256 amountIn, bytes memory swapCalldata) internal returns (uint256 amountOut) {
address tokenOut = fastTransferGateway.token();
Copy link
Collaborator

@dhfang dhfang Nov 9, 2024

Choose a reason for hiding this comment

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

Its a little strange that the caller needs to match their swapCallData to swap into a token whose address is stored in the gateway. This is probably fine in practice since we are generating the swap payload, but if we mess up, can we run into situations where funds are stuck if the swap output token in the swapCallData does not match the gateway token?

uint64 timeoutTimestamp,
bytes calldata destinationCalldata
) public payable returns (bytes32) {
require(executionFeeAmount != 0, "execution fee cannot be zero");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure this a safe assumption to make. @NotJeremyLiu

Copy link
Member

Choose a reason for hiding this comment

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

If this is the source + dest gas fee, then I think there could be cases where it is 0

Copy link
Collaborator

@dhfang dhfang Nov 11, 2024

Choose a reason for hiding this comment

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

@thal0x if we remove this check i think we have a potential fund loss bug if the swapCallData ouput denom does not match the fast transfer gateway asset as the check in line 54 can pass on a zero swapAmountOut. can we rewrite things to avoid that possibility here?

it also seems like we should just make the contract upgradeable - seems like cheap insurance against stuck funds in general. wdyt?

AxelarHandler/src/GoFastHandler.sol Show resolved Hide resolved
Copy link
Collaborator

@dhfang dhfang left a comment

Choose a reason for hiding this comment

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

new changes look good to me ty sir. im assuming upgradeability will be pretty standard? re request review if not

@thal0x thal0x merged commit 2da3240 into main Nov 13, 2024
1 check failed
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.

3 participants