Gbi 2845/discovery contract implementation#356
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function callRegister( | ||
| address discovery, | ||
| string calldata name, | ||
| string calldata apiBaseUrl, | ||
| bool status, | ||
| Flyover.ProviderType providerType | ||
| ) external payable { | ||
| IFlyoverDiscovery(discovery).register{value: msg.value}( | ||
| name, | ||
| apiBaseUrl, | ||
| status, | ||
| providerType | ||
| ); | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium test
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
Luisfc68
left a comment
Luisfc68
left a comment
There was a problem hiding this comment.
Mostly looks good, there are a couple things to change:
- Move some parts to
CollateralManagement - Send the money received in register to
CollateralManagement
Also, we might want to add a way for the different parties to verify which are the addresses of the other contracts, we can discuss later if this is the proper contract to do so or not
|
|
||
| if (providerType > type(Flyover.ProviderType).max) revert InvalidProviderType(providerType); | ||
|
|
||
| if (_resignationBlockNum[providerAddress] != 0) { |
There was a problem hiding this comment.
this condition and the ones below should change since the minCollateral doesn't go on discovery, you can check here for reference https://github.com/rsksmart/liquidity-bridge-contract/blob/lbc-split/contracts/split/FlyoverDiscovery.sol#L138
There was a problem hiding this comment.
I have removed minCollateral check and for the AlreadyRegistered condition, I think we could simply use _collateralManagement.isRegistered function instead of what is done in the PoC.
There was a problem hiding this comment.
Slither found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…erDiscovery contracts to use it
Co-authored-by: Luisfc68 <60527258+Luisfc68@users.noreply.github.com>
Co-authored-by: Luisfc68 <60527258+Luisfc68@users.noreply.github.com>
…te variables and functions
… for liquidity providers and sending assets to collateral contract
…rit documentation
Hakob23
force-pushed
the
GBI-2845/discovery-contract-implementation
branch
from
9b08981 to
c017180
Compare
| function _shouldBeListed(Flyover.LiquidityProvider storage lp) private view returns(bool){ | ||
| return _collateralManagement.isRegistered(lp.providerType, lp.providerAddress) && lp.status; | ||
| } |
Check notice
Code scanning / Slither
Calls inside a loop Low
| status: status, | ||
| providerType: providerType | ||
| }); | ||
| _addCollateral(providerType, msg.sender, msg.value); |
There was a problem hiding this comment.
I think you should emit Register before, because _addCollateral has an external call inside
| /// @notice Lists LPs that should be visible to users | ||
| /// @dev A provider is listed if it has sufficient collateral for at least one side and `status` is true | ||
| /// @return providersToReturn Array of LP records to display | ||
| function getProviders() external view returns (Flyover.LiquidityProvider[] memory) { |
There was a problem hiding this comment.
I maintain this comment from the previous review
Co-authored-by: Luisfc68 <60527258+Luisfc68@users.noreply.github.com>
…ry gas cost in case of tx fail
…ng initialization
…lateral management fixture
Luisfc68
left a comment
Luisfc68
left a comment
There was a problem hiding this comment.
besides the isOperational comment the rest seems to be ok
…d more operational checks for liquidity providers
3 participants
What
Why
To finalize with the implementation of the split contracts
Task
https://rsklabs.atlassian.net/browse/GBI-2845