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

[WIP] UMIP-179: Updates #602

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

[WIP] UMIP-179: Updates #602

wants to merge 10 commits into from

Conversation

pxrl
Copy link
Contributor

@pxrl pxrl commented Apr 22, 2024

  • Truncate LP fee precision to 8 (tbd) decimals.
  • Remove deprecated legacy v3 events.
  • Add link to OZ audit reports.

- Specify proposer action on invalid FilledV3Relay repaymentChainId.
- Clarify proposer action on un-executable RelayerRefundLeaf.
- Add link to OZ audit for Across V3.
@pxrl pxrl requested review from nicholaspai and mrice32 April 22, 2024 10:03
Comment on lines 260 to 263
If a validated `FilledV3Relay` event specifies an invalid `repaymentChainId`, the proposer shall issue repayment on the fill destination chain.
Reasons for the `repaymentChainId` to be considered invalid are:
- `repaymentChainId` is not supported by Across.
- `inputToken` is not supported as a repayment token on `repaymentChainId`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think forcing repayment on the destination chain is the only safe approach here, since if the relayer fills via a contract on zkSync and the proposer proposes repayment on some other chain then the relayer will never receive their repayment.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point. I think that's right.

- The LP fee is typically referenced as a multiplier of the `V3FundsDeposited` `inputAmount`, named `realizedLpFeePct` elsewhere in this document.
- The LP fee as a multiplier of `inputAmount` (typically named `realizedLpFeePct` elsewhere in this document) shall be:
- Promoted to 18 decimals, where 1e18 represents 100%, and
- Truncated (zeroed) between 0 and 1e10, providing 8 decimals of effective precision.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would appreciate feedback on the wording here.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's easier to understand if you show the math?

The LP fee is truncated via the following formula: output = input / 1e10 * 1e10

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

 - Available precision to 8 decimals. Truncated via [output = input / 1e10 * 1e10]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated: 518c7ec

@pxrl pxrl requested review from james-a-morris and bmzig April 22, 2024 14:20
Copy link
Member

@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.

Looks good! A few minor comments. Only issue I see is that one part is ambiguous.

- The LP fee is typically referenced as a multiplier of the `V3FundsDeposited` `inputAmount`, named `realizedLpFeePct` elsewhere in this document.
- The LP fee as a multiplier of `inputAmount` (typically named `realizedLpFeePct` elsewhere in this document) shall be:
- Promoted to 18 decimals, where 1e18 represents 100%, and
- Truncated (zeroed) between 0 and 1e10, providing 8 decimals of effective precision.
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's easier to understand if you show the math?

The LP fee is truncated via the following formula: output = input / 1e10 * 1e10


### Computing Bundle LP Fees
The bundle LP fee for a target block range on a SpokePool and token pair shall be determined by summing the applicable LP fees for each of the following validated events:
- `FilledV3Relay`

### Computing Relayer Repayments
For a validated `FilledV3Relay` event, the relayer repayment amount shall be computed as follows:
- `(inputAmount * (1 - realizedLpFeePct)) / 1e18`, where `realizedLpFeePct` is computed over the set of HubPool `l1Token`, `originChainId` and `repaymentChainId` at the HubPool block number corresponding to the relevant `V3FundsDeposited` `quoteTimestamp`.
- `(inputAmount * (1e18 - realizedLpFeePct)) / 1e18`, where `realizedLpFeePct` is computed over the set of HubPool `l1Token`, `originChainId` and `repaymentChainId` at the HubPool block number corresponding to the relevant `V3FundsDeposited` `quoteTimestamp`.
Copy link
Member

Choose a reason for hiding this comment

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

+1

Comment on lines 260 to 263
If a validated `FilledV3Relay` event specifies an invalid `repaymentChainId`, the proposer shall issue repayment on the fill destination chain.
Reasons for the `repaymentChainId` to be considered invalid are:
- `repaymentChainId` is not supported by Across.
- `inputToken` is not supported as a repayment token on `repaymentChainId`.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, good point. I think that's right.

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.

two possible copy changes

- The LP fee is typically referenced as a multiplier of the `V3FundsDeposited` `inputAmount`, named `realizedLpFeePct` elsewhere in this document.
- The LP fee as a multiplier of `inputAmount` (typically named `realizedLpFeePct` elsewhere in this document) shall be:
- Promoted to 18 decimals, where 1e18 represents 100%, and
- Truncated (zeroed) between 0 and 1e10, providing 8 decimals of effective precision.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

 - Available precision to 8 decimals. Truncated via [output = input / 1e10 * 1e10]


Note:
- If a particular relayer refund is known to be unexecutable, it can be removed from the bundle by the proposer if a sufficient public justification is made before the proposal. This is intended to deal with unlikely situations, such as ag centralized token issuer blacklisting an address that is due a refund. If this leaf were to remain unaltered, this blacklisted address could block other addresses from recieving refunds.
Across v3 has been [audited by OpenZeppelin](https://blog.openzeppelin.com/across-v3-incremental-audit).
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
Across v3 has been [audited by OpenZeppelin](https://blog.openzeppelin.com/across-v3-incremental-audit).
Across v3 has been audited by OpenZeppelin, shared [here](https://blog.openzeppelin.com/across-v3-incremental-audit) and [here](https://blog.openzeppelin.com/across-v3-and-oval-incremental-audit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. It looks like OZ haven't posted about their most recent audit yet. @mrice32 are we able to prompt on that?

@pxrl pxrl requested a review from grasphoper March 6, 2025 11:15
@pxrl pxrl changed the title [WIP] [fix]: Address UMIP-179 feedback [WIP] UMIP-179: Updates Mar 6, 2025
@@ -407,18 +360,19 @@ Each valid `Fill` is subject to an LP fee. The procedure for computing LP fees i
- The `HubPool` `liquidityUtilizationCurrent()` and `liquidityUtilizationPostRelay()` functions shall be used instead of the `BridgePool` variant.
- The event `inputToken` shall be mapped from the SpokePool address to a HubPool `l1Token` address by following the matching procedure outlined above.
- The LP fee is computed between the `originChainId` specified by the `Deposit` and `repaymentChainId` specified by the relayer, where the `relayExecutionInfo.FillType != SlowFill` and the Fill `destinationChainId` otherwise.
- The LP fee as a multiplier of `inputAmount` (typically named `realizedLpFeePct` elsewhere in this document) shall be:
- Promoted to 18 decimals, where 1e18 represents 100%, and
- Truncated to 8 decimals of effective precision, as follows: lpFee = (inputAmount / 1e10) * 1e10
Copy link
Member

Choose a reason for hiding this comment

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

Why 8? why not 6? Just curious how you selected 8, as I imagine the fewest # of precision decimals we can use -- the better UX

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.

4 participants