Skip to content

Feature/GBI-2806 - Discovery & Collateral Management#340

Merged
Luisfc68 merged 8 commits intolbc-splitfrom
feature/GBI-2806
Jul 24, 2025
Merged

Feature/GBI-2806 - Discovery & Collateral Management#340
Luisfc68 merged 8 commits intolbc-splitfrom
feature/GBI-2806

Conversation

@Luisfc68
Copy link
Copy Markdown
Collaborator

What

  • Update solidity compiler version of the contracts to 0.8.25
  • Update hardhat version to 2.25.0
  • Update OZ contracts version to 5.3.0
  • Remove some getters from LiquidityBridgeContractV2 to be able to fit the max size with the new version of the OZ contracts
  • Added 2 versions of Flyover Discovery split (version with everything in the same contract and version with one contract for Discovery and one contract for CollateralManagement)
  • Added test executing all the functions where Discovery and CollateralManagement interact
    • register
    • isOperational
    • getProviders
  • Added gas report of the execution of the previously mentioned test from 1 to 5 liquidity providers in the contract

What is NOT in this PR and will be added later

  • Linter adjustments for the new contracts (will be added once we decide on the version)
  • NatSpec documentation for the new contracts (will be added once we decide on the version)
  • Tests for the new contracts (the current test suite will be adapted to run in the split version in a separate task, also once we decide on the implementation that contract will have its own suite)

The purpose of this PR is only to gather information to decide if we go with 3 or 4 contracts

Task

https://rsklabs.atlassian.net/browse/GBI-2806

Comment on lines +112 to +128
function withdrawCollateral() external nonReentrant {
address providerAddress = msg.sender;
uint resignationBlock = _resignationBlockNum[providerAddress];
if (resignationBlock <= 0) revert NotResigned(providerAddress);
if (block.number - resignationBlock < _resignDelayInBlocks) {
revert ResignationDelayNotMet(providerAddress, resignationBlock, _resignDelayInBlocks);
}

uint amount = _pegOutCollateral[providerAddress] + _pegInCollateral[providerAddress];
_pegOutCollateral[providerAddress] = 0;
_pegInCollateral[providerAddress] = 0;
_resignationBlockNum[providerAddress] = 0;

emit WithdrawCollateral(providerAddress, amount);
(bool success,) = providerAddress.call{value: amount}("");
if (!success) revert WithdrawalFailed(providerAddress, amount);
}

Check warning

Code scanning / Slither

Low-level calls Warning

}

function withdrawCollateral() external nonReentrant {
address providerAddress = msg.sender;

Check notice

Code scanning / Slither

Missing zero address validation Low

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 10, 2025

Dependency Review

The following issues were found:

  • ✅ 0 vulnerable package(s)
  • ✅ 0 package(s) with incompatible licenses
  • ✅ 0 package(s) with invalid SPDX license definitions
  • ✅ 0 package(s) with unknown licenses.
  • ⚠️ 5 packages with OpenSSF Scorecard issues.

View full job summary

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates core dependencies and compiler settings, introduces a split between discovery and collateral management contracts (along with benchmarks and gas reports), and adapts tests to the new APIs.

  • Bump Solidity compiler to 0.8.25, Hardhat to 2.25.0, and OpenZeppelin contracts to v5.3.0; remove some legacy getters.
  • Add split implementations (FlyoverDiscoveryFull, FlyoverDiscoveryContract, CollateralManagementContract) and related interfaces.
  • Introduce a benchmark test with gas reporting and update existing tests to use new contract methods.

Reviewed Changes

Copilot reviewed 26 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/utils/fixtures.ts Switched from getBridgeAddress() to auto-generated .bridge()
test/pegin.test.ts Updated hardcoded addresses for pegin test cases
test/discovery.test.ts Replaced getProviderIds() with the new .providerId() getter
test/benchmark.test.ts Added a comprehensive benchmark suite with console logging
package.json Bumped devDependencies and dependencies versions
hardhat.config.ts Updated Solidity version and added gasReporter configuration
e2e/multisig-migration.test.ts Switched to upgradeAndCall in upgrade tests
docs/discovery-report.md New gas usage report for different LP counts
contracts/split/interfaces.sol New shared types and errors for split contracts
contracts/split/FlyoverDiscoveryFull.sol Full discovery+collateral contract
contracts/split/FlyoverDiscovery.sol Discovery-only contract
contracts/split/CollateralManagement.sol Collateral management contract
contracts/**/*.sol Updated pragma to 0.8.25 across all Solidity files
.solhint.json Updated compiler-version rule to 0.8.25
Comments suppressed due to low confidence (2)

test/benchmark.test.ts:181

  • This benchmark logs the output but has no assertions against expected values; consider adding checks to validate behavior rather than relying solely on console output.
    const discoveryProviders = await discovery.getProviders();

test/discovery.test.ts:27

  • [nitpick] Using a method named providerId() may be unclear—consider renaming the underlying state variable or providing a more descriptive getter like lastProviderId() for consistency.
    const lastProviderId = await lbc.providerId();

@Luisfc68 Luisfc68 marked this pull request as ready for review July 10, 2025 18:40
@Luisfc68 Luisfc68 deleted the branch lbc-split July 16, 2025 16:56
@Luisfc68 Luisfc68 closed this Jul 16, 2025
@Luisfc68 Luisfc68 reopened this Jul 17, 2025
@Luisfc68 Luisfc68 changed the base branch from QA-Test to lbc-split July 24, 2025 19:29
@Luisfc68 Luisfc68 merged commit 647b5e3 into lbc-split Jul 24, 2025
4 checks passed
@Luisfc68 Luisfc68 deleted the feature/GBI-2806 branch July 24, 2025 19:43
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