feat(hubPool): Add sync method and liquidity utilization #26
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
chrismaree
commented
Feb 8, 2022
chrismaree
commented
- nit
- nit
- nit
- Update contracts/HubPool.sol
- review nit
- review nit
- feat(hubpool): Refactor hub pool to use 1D bitmap integer
- nit
- WIP
- review nit
- review nit
- nit
- nit
- nit
- nit
- nit
- nit
- WIP
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- WIP
- nit
- nit
- WIP
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- nit
- feat(slack-config): Add sync method and liquidity utilization
Co-authored-by: Matt Rice <matthewcrice32@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
Signed-off-by: chrismaree <christopher.maree@gmail.com>
chrismaree
changed the title
chrismaree
commented
Feb 8, 2022
| struct PooledToken { | ||
| address lpToken; | ||
| bool isEnabled; | ||
| uint256 liquidReserves; |
Member
Author
There was a problem hiding this comment.
re-order for better packing.
chrismaree
commented
Feb 8, 2022
| } | ||
|
|
||
| // Added to enable the BridgePool to receive ETH. used when unwrapping Weth. | ||
| function _sync(address l1Token) internal { |
Member
Author
There was a problem hiding this comment.
this is numerically identical to v1.
nicholaspai
reviewed
Feb 8, 2022
|
|
||
| emit WhitelistRoute(destinationChainId, originToken, destinationToken); | ||
|
|
||
| // TODO: Should relay message to L2 for destinationChainId and call setEnableRoute(originToken, destinationChainId, true) |
Member
There was a problem hiding this comment.
why remove comment? is this TODO resolved?
Member
Author
There was a problem hiding this comment.
mistake in merging. added back.
nicholaspai
reviewed
Feb 8, 2022
|
|
||
| ExpandedIERC20(pooledTokens[l1Token].lpToken).burnFrom(msg.sender, lpTokenAmount); | ||
| // Note this method does not make any liquidity utilization checks before letting the LP redeem their LP tokens. | ||
| // If they try access more funds that available (i.e l1TokensToReturn > liquidReserves) this will underflow. |
Member
There was a problem hiding this comment.
this underflow is expected + fine in this case right?
Member
Author
There was a problem hiding this comment.
yup! solidity 8 protects against underflows so this simply reverts.
Contributor
There was a problem hiding this comment.
Is it possible for this to underflow, but the contract to have enough tokens to transfer out?
nicholaspai
reviewed
Feb 8, 2022
contracts/HubPool.sol
Outdated
| return _liquidityUtilizationPostRelay(l1Token, 0); | ||
| } | ||
|
|
||
| function liquidityUtilizationPostRelay(address l1Token, uint256 relayedAmount) public returns (uint256) { |
Member
There was a problem hiding this comment.
nit make this nonReentrant also
nicholaspai
reviewed
Feb 8, 2022
mrice32
approved these changes
Feb 9, 2022
|
|
||
| ExpandedIERC20(pooledTokens[l1Token].lpToken).burnFrom(msg.sender, lpTokenAmount); | ||
| // Note this method does not make any liquidity utilization checks before letting the LP redeem their LP tokens. | ||
| // If they try access more funds that available (i.e l1TokensToReturn > liquidReserves) this will underflow. |
Contributor
There was a problem hiding this comment.
Is it possible for this to underflow, but the contract to have enough tokens to transfer out?
pxrl
added a commit
that referenced
this pull request
Feb 8, 2024
This is another case of a variable being missed when the naming was changed. The data availability layer isn't bound to IPFS and will probably be something else anyway. This is within the scope of the UMIP, the contracts don't need to be specific about the underlying technology that is used.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.