-
Notifications
You must be signed in to change notification settings - Fork 56
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
refactor: Consolidate relayer fillRelay method #967
Conversation
No change in behaviour, but a but less code duplication between the normal and sped-up fillRelay code paths. This is a precursor to supporting messaging.
? `Filled deposit ${messageModifier}🚀` | ||
: `Partially filled deposit ${messageModifier}📫"`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to call this out specifically, since the wording of the log message is changed by this PR.
Before: Relay instantly sent
& Instantly completed relay
After: Filled deposit
& Partially filled deposit
The rationale for this is that I always found Instantly completed relay
to be confusing - it indicates a partial fill, but the only real indicator of this is the postbox emoji (📫). RE Relay instantly sent
, this can also occur on a deposit that is hours or days old, so instantly
is pretty subjective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! +1 on updating the log message
this.logger.debug({ at: "Relayer", message: "Filling deposit", deposit, repaymentChainId }); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - this is clean and I don't have anything actionable to recommend
const srcChain = getNetworkName(deposit.originChainId); | ||
const dstChain = getNetworkName(deposit.destinationChainId); | ||
const amount = createFormatFunction(2, 4, false, decimals)(deposit.amount.toString()); | ||
const depositor = blockExplorerLink(deposit.depositor, deposit.originChainId); | ||
const _fillAmount = createFormatFunction(2, 4, false, decimals)(fillAmount.toString()); | ||
const relayerFeePct = formatFeePct(deposit.relayerFeePct); | ||
const realizedLpFeePct = formatFeePct(deposit.realizedLpFeePct); | ||
return ( | ||
`Relayed depositId ${deposit.depositId} from ${getNetworkName(deposit.originChainId)} ` + | ||
`to ${getNetworkName(deposit.destinationChainId)} of ` + | ||
`${createFormatFunction(2, 4, false, decimals)(deposit.amount.toString())} ${symbol}. ` + | ||
`with depositor ${blockExplorerLink(deposit.depositor, deposit.originChainId)}. ` + | ||
`Fill amount of ${createFormatFunction(2, 4, false, decimals)(fillAmount.toString())} ${symbol} with ` + | ||
`relayerFee ${formatFeePct(deposit.relayerFeePct)}% & ` + | ||
`realizedLpFee ${formatFeePct(deposit.realizedLpFeePct)}%. ` | ||
`Relayed depositId ${deposit.depositId} from ${srcChain} to ${dstChain} of ${amount} ${symbol},` + | ||
` with depositor ${depositor}. Fill amount of ${_fillAmount} ${symbol} with` + | ||
` relayerFee ${relayerFeePct}% & realizedLpFee ${realizedLpFeePct}%.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
No change in behaviour, but a but less code duplication between the normal and sped-up fillRelay code paths.
This is a precursor to supporting messaging.