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

fix(across): address issue in liquidity utilization calculation #3499

Merged
merged 21 commits into from
Oct 26, 2021

Conversation

chrismaree
Copy link
Member

@chrismaree chrismaree commented Oct 26, 2021

Motivation

This PR addresses a small issue in the liquidityUtilizationPostRelay which did not correctly consider the denominator. The denominator now considers utilizedReserves to correctly capture the funds in the pool.

liquidityUtilizationRatio := (relayedAmount + pendingReserves + utilizedReserves) / (liquidReserves + utilizedReserves)

Testing

Check a box to describe how you tested these changes and list the steps for reviewers to test.

  • Ran end-to-end test, running the code as in production
  • New unit tests created
  • Existing tests adequate, no new tests required
  • All existing tests pass
  • Untested

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
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 great!

@chrismaree chrismaree added the insured-bridge Optimism-Ethereum insured bridge contracts label Oct 26, 2021
nicholaspai and others added 4 commits October 26, 2021 15:48
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
chrismaree and others added 3 commits October 26, 2021 18:42
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
chrismaree and others added 3 commits October 26, 2021 18:46
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
chrismaree and others added 4 commits October 26, 2021 18:48
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Co-authored-by: nicholaspai <9457025+nicholaspai@users.noreply.github.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
…github.com:UMAprotocol/protocol into chrismaree/fix-implementation-liquidityutilization
@chrismaree chrismaree marked this pull request as ready for review October 26, 2021 18:00
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

LGTM awesome work

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>
_sync(); // Fetch any balance changes due to token bridging finalization and factor them in.

// liquidityUtilizationRatio :=
// (relayedAmount + pendingReserves + max(utilizedReserves,0)) / (liquidReserves + max(utilizedReserves,0))
Copy link
Contributor

Choose a reason for hiding this comment

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

One side effect of flooring utilizedReserves when it is negative (due to having tokens transferred to the contract by mistake) is that the calculated utilization rate would drop just after the pending relay is settled since the settlement moves deposit amount from pendingReserves to utilizedReserves.

Maybe alternative to avoid this could be to floor the sum of (pendingReserves + utilizedReserves)? Though not sure if this still is ideal, but the source of the problem is not being able to distinguish token transfers by mistake from L2->L1 native bridging transfers.

Copy link
Member Author

Choose a reason for hiding this comment

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

ye, this is a good idea! I think we'll leave as it is for now and can re-visit this in the coming week and make sure that we are all happy with how this is implemented right now.

@chrismaree chrismaree merged commit 35e279a into master Oct 26, 2021
@chrismaree chrismaree deleted the chrismaree/fix-implementation-liquidityutilization branch December 19, 2022 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
insured-bridge Optimism-Ethereum insured bridge contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants