Conversation
…in system handlers
…ction across integration tests
…and withdrawal scenarios
…n DeployFlyover tests
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function _refundCollateral(address payable providerAddress, uint256 amount) private { | ||
| (bool success,) = providerAddress.call{value: amount}(""); | ||
| if (!success) revert Flyover.PaymentFailed(providerAddress, amount, hex""); | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF Scorecard
Scanned Files |
|
|
||
| mapping(uint => Flyover.LiquidityProvider) private _liquidityProviders; | ||
| mapping(address => uint) private _providerIdByAddress; | ||
| mapping(address => PendingRegistration) private _pendingRegistrations; |
There was a problem hiding this comment.
This will be deployed in already existing contracts (since the 2.5.0 is already deployed) so we need to respect the storage layout from now on. This mappings should be moved to below the already existing variables
| if (_registrationStates[msg.sender] != RegistrationState.Pending) { | ||
| revert RegistrationNotPending(msg.sender); | ||
| } | ||
| uint256 providerId = _providerIdByAddress[msg.sender]; |
There was a problem hiding this comment.
which is the rationale to give the provider id when the registration request is created? isn't it more intuitive (and simple) to give it only if the registration is approved?
|
|
||
| /// @inheritdoc IFlyoverDiscovery | ||
| function rejectRegistration(address providerAddress) external whenNotPaused { | ||
| if (msg.sender != defaultAdmin()) revert NotAuthorized(msg.sender); |
There was a problem hiding this comment.
the admin check and pending state check repeats in all the registration management functions, maybe we should add a modifier, do you see any downsides?
| function _getProvider(address providerAddress) private view returns (Flyover.LiquidityProvider memory) { | ||
| uint providerId = _providerIdByAddress[providerAddress]; | ||
| if (providerId == 0) revert Flyover.ProviderNotRegistered(providerAddress); | ||
| if (providerId == 0 || _registrationStates[providerAddress] != RegistrationState.Approved) { |
There was a problem hiding this comment.
For example, I think this change wouldn't be necessary if we granted the id only after the approval
3 participants
What
Implement a two-step LP registration flow.
Added request lifecycle functions
approveRegistration,rejectRegistration, andwithdrawRegisterRequest, plusgetRegistrationStategetter function, tests and fixture updates.Why
This change prevents immediate activation on
registerand guarantees collateral is returned when a request is rejected or withdrawn.Type of Change
Affected Areas
registerPegIn,callForUser, quote validation)depositPegout,refundPegOut,refundUserPegOut)ci,fuzz,slither,codeql)Risk & Security Review
Additional notes on test coverage, edge cases, and any tests intentionally skipped:
forge test --match-path 'test/discovery/*.t.sol'forge test --match-path 'test/integration/*.t.sol'forge test --match-path test/Pause.t.solforge test --match-path test/Benchmark.t.solforge test(full suite)Deployment & Ops Impact
.env, network RPCs, verification, etc.)Related Issues
Key local outputs:
forge testsuite passed after updating registration-dependent tests and fixtures.