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

refactor: Consolidate relayer fillRelay method #967

Merged
merged 3 commits into from
Oct 5, 2023
Merged

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Oct 4, 2023

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.

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.
Comment on lines +318 to +319
? `Filled deposit ${messageModifier}🚀`
: `Partially filled deposit ${messageModifier}📫"`;
Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@mrice32 mrice32 left a 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 });

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

@james-a-morris james-a-morris left a 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

Comment on lines +690 to +700
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}%.`
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@pxrl pxrl merged commit 07cb73e into master Oct 5, 2023
@pxrl pxrl deleted the pxrl/fillRelay branch October 5, 2023 13:16
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