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.

Sign up for GitHub

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: precision issue #493

Merged
merged 5 commits into from
Nov 21, 2024
Merged

fix: precision issue #493

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c2a4683
fix: precision issue
Mikelle Nov 20, 2024
8a82a64
fix: fixed oracle tests
Mikelle Nov 20, 2024
32fbd28
Merge branch 'main' into fix/492/precision-issue
Mikelle Nov 21, 2024
e7e2440
fix: introduce constant in tests
Mikelle Nov 21, 2024
19aa419
fix: rename
Mikelle Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 8 additions & 21 deletions contracts-abi/abi/BidderRegistry.abi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,6 @@
],
"stateMutability": "view"
},
{
"type": "function",
"name": "PRECISION",
"inputs": [],
"outputs": [
{
"name": "",
"type": "uint256",
"internalType": "uint256"
}
],
"stateMutability": "view"
},
{
"type": "function",
"name": "UPGRADE_INTERFACE_VERSION",
Expand Down Expand Up @@ -152,8 +139,8 @@
"outputs": [
{
"name": "",
"type": "uint16",
"internalType": "uint16"
"type": "uint256",
"internalType": "uint256"
}
],
"stateMutability": "view"
Expand Down Expand Up @@ -225,8 +212,8 @@
},
{
"name": "_feePercent",
"type": "uint16",
"internalType": "uint16"
"type": "uint256",
"internalType": "uint256"
},
{
"name": "_owner",
Expand Down Expand Up @@ -516,8 +503,8 @@
"inputs": [
{
"name": "newFeePercent",
"type": "uint16",
"internalType": "uint16"
"type": "uint256",
"internalType": "uint256"
}
],
"outputs": [],
Expand Down Expand Up @@ -755,9 +742,9 @@
"inputs": [
{
"name": "newFeePercent",
"type": "uint16",
"type": "uint256",
"indexed": true,
"internalType": "uint16"
"internalType": "uint256"
}
],
"anonymous": false
Expand Down
2 changes: 1 addition & 1 deletion contracts-abi/abi/Oracle.abi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@
},
{
"type": "error",
"name": "ResidualBidPercentAfterDecayExceeds100",
"name": "ResidualBidPercentAfterDecayExceedsMax",
"inputs": [
{
"name": "residualBidPercentAfterDecay",
Expand Down
29 changes: 8 additions & 21 deletions contracts-abi/abi/ProviderRegistry.abi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,6 @@
],
"stateMutability": "view"
},
{
"type": "function",
"name": "PRECISION",
"inputs": [],
"outputs": [
{
"name": "",
"type": "uint256",
"internalType": "uint256"
}
],
"stateMutability": "view"
},
{
"type": "function",
"name": "UPGRADE_INTERFACE_VERSION",
Expand Down Expand Up @@ -158,8 +145,8 @@
"outputs": [
{
"name": "",
"type": "uint16",
"internalType": "uint16"
"type": "uint256",
"internalType": "uint256"
}
],
"stateMutability": "view"
Expand Down Expand Up @@ -250,8 +237,8 @@
},
{
"name": "_feePercent",
"type": "uint16",
"internalType": "uint16"
"type": "uint256",
"internalType": "uint256"
},
{
"name": "_owner",
Expand Down Expand Up @@ -495,8 +482,8 @@
"inputs": [
{
"name": "newFeePercent",
"type": "uint16",
"internalType": "uint16"
"type": "uint256",
"internalType": "uint256"
}
],
"outputs": [],
Expand Down Expand Up @@ -705,9 +692,9 @@
"inputs": [
{
"name": "newFeePercent",
"type": "uint16",
"type": "uint256",
"indexed": true,
"internalType": "uint16"
"internalType": "uint256"
}
],
"anonymous": false
Expand Down
103 changes: 36 additions & 67 deletions contracts-abi/clients/BidderRegistry/BidderRegistry.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion contracts-abi/clients/Oracle/Oracle.go
View file
Open in desktop

Large diffs are not rendered by default.

103 changes: 36 additions & 67 deletions contracts-abi/clients/ProviderRegistry/ProviderRegistry.go
View file
Open in desktop

Large diffs are not rendered by default.

9 changes: 4 additions & 5 deletions contracts/contracts/core/BidderRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ contract BidderRegistry is
*/
function initialize(
address _protocolFeeRecipient,
uint16 _feePercent,
uint256 _feePercent,
address _owner,
address _blockTracker,
uint256 _feePayoutPeriodBlocks
Expand Down Expand Up @@ -184,10 +184,9 @@ contract BidderRegistry is
require(bidState.state == State.PreConfirmed, BidNotPreConfirmed(commitmentDigest, bidState.state, State.PreConfirmed));

uint256 decayedAmt = (bidState.bidAmt *
residualBidPercentAfterDecay *
PRECISION) / PERCENT;
residualBidPercentAfterDecay) / PERCENT;

uint256 feeAmt = (decayedAmt * uint256(feePercent) * PRECISION) /
uint256 feeAmt = (decayedAmt * feePercent) /
PERCENT;
uint256 amtMinusFeeAndDecay = decayedAmt - feeAmt;

Expand Down Expand Up @@ -306,7 +305,7 @@ contract BidderRegistry is
* @dev onlyOwner restriction
* @param newFeePercent this is the new fee percent
*/
function setNewFeePercent(uint16 newFeePercent) external onlyOwner {
function setNewFeePercent(uint256 newFeePercent) external onlyOwner {
feePercent = newFeePercent;
emit FeePercentUpdated(newFeePercent);
}
Expand Down
5 changes: 2 additions & 3 deletions contracts/contracts/core/BidderRegistryStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,13 @@ abstract contract BidderRegistryStorage {
using FeePayout for FeePayout.Tracker;

/// @dev For improved precision
uint256 constant public PRECISION = 1e25;
uint256 constant public PERCENT = 100 * PRECISION;
uint256 constant public PERCENT = 1e18;
ckartik marked this conversation as resolved.
Show resolved Hide resolved

/// @dev Address of the preconfManager contract
address public preconfManager;

/// @dev Fee percent that would be taken by protocol when provider is slashed
uint16 public feePercent;
uint256 public feePercent;

/// @dev Block tracker contract
IBlockTracker public blockTrackerContract;
Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/core/Oracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ contract Oracle is OracleStorage, IOracle, Ownable2StepUpgradeable, UUPSUpgradea
) external onlyOracle whenNotPaused {
address blockWinner = _blockTrackerContract.getBlockWinner(blockNumber);
require(blockWinner == builder, BuilderNotBlockWinner(blockWinner, builder));
require(residualBidPercentAfterDecay <= 100, ResidualBidPercentAfterDecayExceeds100(residualBidPercentAfterDecay));
require(residualBidPercentAfterDecay <= 1e18, ResidualBidPercentAfterDecayExceedsMax(residualBidPercentAfterDecay));

IPreconfManager.OpenedCommitment
memory commitment = _preconfManager.getCommitment(commitmentIndex);
Expand Down
8 changes: 4 additions & 4 deletions contracts/contracts/core/ProviderRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ contract ProviderRegistry is
function initialize(
uint256 _minStake,
address _penaltyFeeRecipient,
uint16 _feePercent,
uint256 _feePercent,
address _owner,
uint256 _withdrawalDelay,
uint256 _penaltyFeePayoutPeriodBlocks
Expand Down Expand Up @@ -104,8 +104,8 @@ contract ProviderRegistry is
address payable bidder,
uint256 residualBidPercentAfterDecay
) external nonReentrant onlyPreconfManager whenNotPaused {
uint256 residualAmt = (amt * residualBidPercentAfterDecay * PRECISION) / PERCENT;
uint256 penaltyFee = (residualAmt * uint256(feePercent) * PRECISION) / PERCENT;
uint256 residualAmt = (amt * residualBidPercentAfterDecay) / PERCENT;
uint256 penaltyFee = (residualAmt * feePercent) / PERCENT;
uint256 providerStake = providerStakes[provider];

if (providerStake < residualAmt + penaltyFee) {
Expand Down Expand Up @@ -153,7 +153,7 @@ contract ProviderRegistry is
* @dev onlyOwner restriction
* @param newFeePercent this is the new fee percent
*/
function setNewFeePercent(uint16 newFeePercent) external onlyOwner {
function setNewFeePercent(uint256 newFeePercent) external onlyOwner {
feePercent = newFeePercent;
emit FeePercentUpdated(newFeePercent);
}
Expand Down
5 changes: 2 additions & 3 deletions contracts/contracts/core/ProviderRegistryStorage.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ abstract contract ProviderRegistryStorage {
using FeePayout for FeePayout.Tracker;

/// @dev For improved precision
uint256 public constant PRECISION = 1e25;
uint256 public constant PERCENT = 100 * PRECISION;
uint256 public constant PERCENT = 1e18;

/// @dev Minimum stake required for registration
uint256 public minStake;
Expand All @@ -17,7 +16,7 @@ abstract contract ProviderRegistryStorage {
address public preconfManager;

/// @dev Fee percent that would be taken by protocol when provider is slashed
uint16 public feePercent;
uint256 public feePercent;

/// @dev Configurable withdrawal delay in milliseconds
uint256 public withdrawalDelay;
Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/interfaces/IBidderRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ interface IBidderRegistry {
event PreconfManagerUpdated(address indexed newPreconfManager);

/// @dev Event emitted when the fee percent is updated
event FeePercentUpdated(uint16 indexed newFeePercent);
event FeePercentUpdated(uint256 indexed newFeePercent);

/// @dev Event emitted when the block tracker is updated
event BlockTrackerUpdated(address indexed newBlockTracker);
Expand Down
4 changes: 2 additions & 2 deletions contracts/contracts/interfaces/IOracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ interface IOracle {
/// @dev Error emitted when the builder is not the block winner
error BuilderNotBlockWinner(address blockWinner, address builder);

/// @dev Error emitted when the residual bid percent after decay exceeds 100
error ResidualBidPercentAfterDecayExceeds100(uint256 residualBidPercentAfterDecay);
/// @dev Error emitted when the residual bid percent after decay exceeds max amount
error ResidualBidPercentAfterDecayExceedsMax(uint256 residualBidPercentAfterDecay);

receive() external payable;

Expand Down
2 changes: 1 addition & 1 deletion contracts/contracts/interfaces/IProviderRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ interface IProviderRegistry {
event PreconfManagerUpdated(address indexed newPreconfManager);

/// @dev Event emitted when the fee percent is updated
event FeePercentUpdated(uint16 indexed newFeePercent);
event FeePercentUpdated(uint256 indexed newFeePercent);

/// @dev Event emitted when there are insufficient funds to slash
event InsufficientFundsToSlash(
Expand Down
4 changes: 2 additions & 2 deletions contracts/scripts/core/DeployCore.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ contract DeployTestnet is Script {
address protocolFeeRecipient = address(
0xfA0B0f5d298d28EFE4d35641724141ef19C05684 // Placeholder for now, L1 preconf.eth address
);
uint16 feePercent = 2;
uint16 providerPenaltyPercent = 5;
uint256 feePercent = 2 * 1e16; // 2%
Copy link
Collaborator

Choose a reason for hiding this comment

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

The number 1e16 repeats, consider creating a constant.

uint256 providerPenaltyPercent = 5 * 1e16; // 5%
uint64 commitmentDispatchWindow = 2000;
uint256 withdrawalDelay = 24 hours * 1000; // 24 hours in milliseconds
uint256 protocolFeePayoutPeriodBlocks = 5 hours ; // 1 hour with 200ms blocks
Expand Down
18 changes: 9 additions & 9 deletions contracts/test/core/BidderRegistryTest.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {ProviderRegistry} from "../../contracts/core/ProviderRegistry.sol";
contract BidderRegistryTest is Test {
uint256 public testNumber;
BidderRegistry public bidderRegistry;
uint16 public feePercent;
uint256 public feePercent;
uint256 public minStake;
address public bidder;
address public feeRecipient;
Expand All @@ -29,7 +29,7 @@ contract BidderRegistryTest is Test {

function setUp() public {
testNumber = 42;
feePercent = 10;
feePercent = 10 * 1e16;
minStake = 1e18 wei;
feeRecipient = vm.addr(9);
feePayoutPeriodBlocks = 100;
Expand Down Expand Up @@ -194,7 +194,7 @@ contract BidderRegistryTest is Test {

bidderRegistry.openBid(commitmentDigest, 1 ether, bidder, blockNumber);

bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), 100);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), 1e18);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't be the 1e18 replaced with PERCENT constant?

uint256 providerAmount = bidderRegistry.providerAmount(provider);
uint256 feeRecipientAmount = bidderRegistry.getAccumulatedProtocolFee();

Expand All @@ -220,7 +220,7 @@ contract BidderRegistryTest is Test {

uint256 bidderBalance = bidder.balance;

bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), 50);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), 5e17);
uint256 providerAmount = bidderRegistry.providerAmount(provider);
uint256 feeRecipientAmount = bidderRegistry.getAccumulatedProtocolFee();

Expand Down Expand Up @@ -269,7 +269,7 @@ contract BidderRegistryTest is Test {
vm.expectRevert(bytes(""));
bytes32 commitmentDigest = keccak256("1234");
bidderRegistry.openBid(commitmentDigest, 1 ether, bidder, blockNumber);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),100);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),1e18);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same as above.

}

function testFail_shouldRetrieveFundsGreaterThanStake() public {
Expand All @@ -287,7 +287,7 @@ contract BidderRegistryTest is Test {
vm.prank(address(this));
bytes32 commitmentDigest = keccak256("1234");
bidderRegistry.openBid(commitmentDigest, 3 ether, bidder, blockNumber);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),100);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),1e18);
Copy link
Collaborator

Choose a reason for hiding this comment

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

The same is here and also in several other places below.

}

function test_withdrawProviderAmount() public {
Expand All @@ -305,7 +305,7 @@ contract BidderRegistryTest is Test {

bidderRegistry.openBid(commitmentDigest, 2 ether, bidder, blockNumber);

bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), 100);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider), 1e18);
bidderRegistry.withdrawProviderAmount(payable(provider));
uint256 balanceAfter = address(provider).balance;
assertEq(balanceAfter - balanceBefore, 1800000000000000000);
Expand Down Expand Up @@ -451,7 +451,7 @@ contract BidderRegistryTest is Test {
bidderRegistry.openBid(commitmentDigest, 1 ether, bidder, blockNumber);
vm.expectEmit(true, true, true, true);
emit FeeTransfer(100000000000000000, feeRecipient);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),100);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),1e18);
uint256 balanceAfter = feeRecipient.balance;
assertEq(balanceAfter - balanceBefore, 100000000000000000);
assertEq(bidderRegistry.getAccumulatedProtocolFee(), 0);
Expand All @@ -470,7 +470,7 @@ contract BidderRegistryTest is Test {
blockTracker.addBuilderAddress("test", provider);
blockTracker.recordL1Block(blockNumber, "test");
bidderRegistry.openBid(commitmentDigest, 1 ether, bidder, blockNumber);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),100);
bidderRegistry.retrieveFunds(nextWindow, commitmentDigest, payable(provider),1e18);
uint256 balanceAfter = feeRecipient.balance;
assertEq(balanceAfter - balanceBefore, 0);
assertEq(bidderRegistry.getAccumulatedProtocolFee(), 100000000000000000);
Expand Down
Loading
Loading