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 contract: new daimo pay relayer #1372

Merged
merged 7 commits into from
Dec 3, 2024
Merged

Conversation

andrewliu08
Copy link
Contributor

  • Changed daimo pay relayer to support multiple owners. This allows us to have separate EOAs to run starts and finishes while still using the same relayer contract.
  • Modified the relayer contract to hold balances for tipping rather than having the relayer EOA hold this balance.
  • Made maxPreTip and maxPostTip separate parameters. This could be useful to avoid unintentionally tipping when we switch to having the relayer hold multiple tokens
  • swapAndTip now emits an event upon success. This will help us track how tipping affects our PnL

Testing

Wrote test suite for new contract which tests:

  1. only the contract creator can withdraw token balances from the relayer contract
  2. swapAndTip pre-tip and post-tip work correctly
  3. swapAndTip doesn't allow pre-tip or post-tip to exceed the max tip amount
  4. excess swap output is kept by the contract

Copy link

vercel bot commented Nov 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
daimo-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 1:56am
daimo-web-stage ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 1:56am

@@ -23,6 +23,10 @@ contract DeployDaimoPayRelayer is Script {

console.log("daimoPayRelayer deployed at address:", daimoPayRelayer);

address relayer = 0x723A63fb50dA50A26997Fb99A2Eb151E4F8c5227;
Copy link
Member

Choose a reason for hiding this comment

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

where does this address come from?

the line below (daimoPayRelayer...grantRelayerRole(relayer)) is a bit confusing

Copy link
Contributor Author

@andrewliu08 andrewliu08 Dec 2, 2024

Choose a reason for hiding this comment

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

this is just the other EOA i created for starts/claims


import "./DaimoPay.sol";
import "./TokenUtils.sol";

/*
* Relayer contract that funds completes DaimoPay intents.
*/
contract DaimoPayRelayer is Ownable2Step {
contract DaimoPayRelayer is AccessControl {
Copy link
Member

Choose a reason for hiding this comment

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

👍

address indexed requiredTokenOut,
uint256 swapAmountOut,
uint256 maxPreTip,
uint256 maxPostTip
Copy link
Member

Choose a reason for hiding this comment

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

might be worth calling this maxIntentTip and maxStartTip for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maxStartTip would be the post-tip and maxIntentTip would be the pre-tip?

personally, i prefer maxPreTip and maxPostTip. i think this naming makes more sense at the function-level context, whereas maxStartTip and maxIntentTip requires global-level context to make sense


// Withdraws the full balance of a token from the contract to the admin.
function withdrawBalance(IERC20 token) public onlyRole(DEFAULT_ADMIN_ROLE) {
TokenUtils.transferBalance(token, payable(msg.sender));
Copy link
Member

Choose a reason for hiding this comment

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

maybe have this return uint256, then return TokenUtils.transferBalance...

this way if admin needs to send the token balance on to anywhere else they know the amount cleanly

Call calldata innerSwap
) external payable {
require(tx.origin == owner(), "DPR: only usable by owner");
require(hasRole(RELAYER_ROLE, tx.origin), "DPR: only relayer");
Copy link
Member

Choose a reason for hiding this comment

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

🚀

// suppliedTokenInAmount. The difference is tipped by the contract. We
// already checked that the tip is within maxPreTip.
if (innerSwap.to != address(0)) {
requiredTokenIn.token.approve(innerSwap.to, requiredTokenIn.amount);
Copy link
Member

Choose a reason for hiding this comment

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

//////////////////////////////////////////////////////////////

// If we received less than required, check that the amount we need to
// tip is within maxPostTip.
if (swapAmountOut < requiredTokenOut.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 if i understand correctly:

  • during start, we have some amount of (random token) and need to produce an exact amount of the bridge token. maxPreTip is 0, maxPostTip is the max amount of the bridge token (eg USDC) we're willing to tip.

  • during fastfinish/claim (whichever is actually running the intent) we have some amount of bridge token and we need an exact amount of destination token. maxPostTip is now 0, and maxPreTip is whatever amount of the bridge token we're willing to tip.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

}

// Transfer the required output tokens to the caller, tipping the
// shortfall if needed. If there are surplus tokens from the swap, keep
// them.
Copy link
Member

Choose a reason for hiding this comment

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

one effect of this is that the relayer will accumulate change in whatever the destination tokens are.

for ethOS, for example, it'll accumulate ETH on Base.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@@ -124,7 +167,7 @@ contract DaimoPayRelayer is Ownable2Step {
Call[] calldata startCalls,
bytes calldata bridgeExtraData,
Call[] calldata postCalls
) public payable onlyOwner {
) public payable onlyRole(RELAYER_ROLE) {
Copy link
Member

Choose a reason for hiding this comment

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

it might be worth renaming this from RELAYER_ROLE / relayer etc to RELAY_EOA_ROLE / relayEOA or similar, to distinguish it from the relayer contract

*/
export function isEvmChain(chainId: number) {
return chainId !== solana.chainId;
}
Copy link
Member

Choose a reason for hiding this comment

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

that's funny, I have this exact diff on another branch.

Copy link
Member

@dcposch dcposch left a comment

Choose a reason for hiding this comment

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

LGreatTM after comments

@andrewliu08 andrewliu08 changed the base branch from nibnalin/fix-leftover to internal December 2, 2024 22:37
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