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

[OZ][N-11] Code Cohesion and Readability #120

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[OZ][N-11] Code Cohesion and Readability #120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 9 additions & 9 deletions contracts/sfc/ConstantsManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ contract ConstantsManager is OwnableUpgradeable {
minSelfStake = v;
}

function updateMaxDelegatedRatio(uint256 v) external virtual onlyOwner {
function updateMaxDelegatedRatio(uint256 v) external onlyOwner {
if (v < Decimal.unit()) {
revert ValueTooSmall();
}
Expand All @@ -71,28 +71,28 @@ contract ConstantsManager is OwnableUpgradeable {
maxDelegatedRatio = v;
}

function updateValidatorCommission(uint256 v) external virtual onlyOwner {
function updateValidatorCommission(uint256 v) external onlyOwner {
if (v > Decimal.unit() / 2) {
revert ValueTooLarge();
}
validatorCommission = v;
}

function updateBurntFeeShare(uint256 v) external virtual onlyOwner {
function updateBurntFeeShare(uint256 v) external onlyOwner {
if (v > Decimal.unit() / 2) {
revert ValueTooLarge();
}
burntFeeShare = v;
}

function updateTreasuryFeeShare(uint256 v) external virtual onlyOwner {
function updateTreasuryFeeShare(uint256 v) external onlyOwner {
if (v > Decimal.unit() / 2) {
revert ValueTooLarge();
}
treasuryFeeShare = v;
}

function updateWithdrawalPeriodEpochs(uint256 v) external virtual onlyOwner {
function updateWithdrawalPeriodEpochs(uint256 v) external onlyOwner {
if (v < 2) {
revert ValueTooSmall();
}
Expand All @@ -102,7 +102,7 @@ contract ConstantsManager is OwnableUpgradeable {
withdrawalPeriodEpochs = v;
}

function updateWithdrawalPeriodTime(uint256 v) external virtual onlyOwner {
function updateWithdrawalPeriodTime(uint256 v) external onlyOwner {
if (v < 86400) {
revert ValueTooSmall();
}
Expand Down Expand Up @@ -132,7 +132,7 @@ contract ConstantsManager is OwnableUpgradeable {
offlinePenaltyThresholdTime = v;
}

function updateOfflinePenaltyThresholdBlocksNum(uint256 v) external virtual onlyOwner {
function updateOfflinePenaltyThresholdBlocksNum(uint256 v) external onlyOwner {
if (v < 100) {
revert ValueTooSmall();
}
Expand Down Expand Up @@ -162,7 +162,7 @@ contract ConstantsManager is OwnableUpgradeable {
gasPriceBalancingCounterweight = v;
}

function updateAverageUptimeEpochWindow(uint32 v) external virtual onlyOwner {
function updateAverageUptimeEpochWindow(uint32 v) external onlyOwner {
if (v < 10) {
// needs to be long enough to allow permissible downtime for validators maintenance
revert ValueTooSmall();
Expand All @@ -173,7 +173,7 @@ contract ConstantsManager is OwnableUpgradeable {
averageUptimeEpochWindow = v;
}

function updateMinAverageUptime(uint64 v) external virtual onlyOwner {
function updateMinAverageUptime(uint64 v) external onlyOwner {
if (v > ((Decimal.unit() * 9) / 10)) {
revert ValueTooLarge();
}
Expand Down
49 changes: 15 additions & 34 deletions contracts/sfc/SFC.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
// redirections
error AlreadyRedirected();
error SameRedirectionAuthorizer();
error Redirected();

// validators
error ValidatorNotExists();
Expand Down Expand Up @@ -205,11 +204,10 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
event RestakedRewards(address indexed delegator, uint256 indexed toValidatorID, uint256 rewards);
event BurntFTM(uint256 amount);
event UpdatedSlashingRefundRatio(uint256 indexed validatorID, uint256 refundRatio);
event RefundedSlashedLegacyDelegation(address indexed delegator, uint256 indexed validatorID, uint256 amount);
event AnnouncedRedirection(address indexed from, address indexed to);

modifier onlyDriver() {
if (!isNode(msg.sender)) {
if (!_isNodeDriverAuth(msg.sender)) {
revert NotDriverAuth();
}
_;
Expand Down Expand Up @@ -342,11 +340,8 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
auth,
validatorID,
pubkey,
OK_STATUS,
0, // createdEpoch
createdTime,
0, // deactivatedEpoch - not deactivated
0 // deactivatedTime - not deactivated
createdTime
);
if (validatorID > lastValidatorID) {
lastValidatorID = validatorID;
Expand Down Expand Up @@ -572,7 +567,7 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
}

/// Check if an address is the NodeDriverAuth contract.
function isNode(address addr) internal view virtual returns (bool) {
function _isNodeDriverAuth(address addr) internal view virtual returns (bool) {
return addr == address(node);
}

Expand Down Expand Up @@ -782,11 +777,6 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
return getEpochSnapshot[epoch].endTime;
}

/// Check if an address is redirected.
function _redirected(address addr) internal view returns (bool) {
return getRedirection[addr] != address(0);
}

/// Get address which should receive rewards and withdrawn stake for the given delegator.
/// The delegator is usually the receiver, unless a redirection is created.
function _receiverOf(address addr) internal view returns (address payable) {
Expand Down Expand Up @@ -826,7 +816,7 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
EpochSnapshot storage prevSnapshot,
uint256[] memory validatorIDs,
uint256[] memory uptimes,
uint256[] memory accumulatedOriginatedTxsFee
uint256[] memory originatedTxsFee
) internal {
SealEpochRewardsCtx memory ctx = SealEpochRewardsCtx(
new uint256[](validatorIDs.length),
Expand All @@ -838,16 +828,16 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {

for (uint256 i = 0; i < validatorIDs.length; i++) {
uint256 prevAccumulatedTxsFee = prevSnapshot.accumulatedOriginatedTxsFee[validatorIDs[i]];
uint256 originatedTxsFee = 0;
if (accumulatedOriginatedTxsFee[i] > prevAccumulatedTxsFee) {
originatedTxsFee = accumulatedOriginatedTxsFee[i] - prevAccumulatedTxsFee;
uint256 accumulatedTxsFee = 0;
if (originatedTxsFee[i] > prevAccumulatedTxsFee) {
accumulatedTxsFee = originatedTxsFee[i] - prevAccumulatedTxsFee;
Copy link
Collaborator

@thaarok thaarok Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is wrong - the txs fee (from txs emitted by given validator) from this epoch is calculated as the difference of the accumulated fee from the current and the accumulated fee from the previous epoch.

Using names originatedTxsFee and accumulatedOriginatedTxsFee was correct in here.

}
// txRewardWeight = {originatedTxsFee} * {uptime}
// originatedTxsFee is roughly proportional to {uptime} * {stake}, so the whole formula is roughly
// {stake} * {uptime} ^ 2
ctx.txRewardWeights[i] = (originatedTxsFee * uptimes[i]) / epochDuration;
ctx.txRewardWeights[i] = (accumulatedTxsFee * uptimes[i]) / epochDuration;
ctx.totalTxRewardWeight = ctx.totalTxRewardWeight + ctx.txRewardWeights[i];
ctx.epochFee = ctx.epochFee + originatedTxsFee;
ctx.epochFee = ctx.epochFee + accumulatedTxsFee;
}

for (uint256 i = 0; i < validatorIDs.length; i++) {
Expand Down Expand Up @@ -889,7 +879,7 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
prevSnapshot.accumulatedRewardPerToken[validatorID] +
rewardPerToken;

snapshot.accumulatedOriginatedTxsFee[validatorID] = accumulatedOriginatedTxsFee[i];
snapshot.accumulatedOriginatedTxsFee[validatorID] = originatedTxsFee[i];
snapshot.accumulatedUptime[validatorID] = prevSnapshot.accumulatedUptime[validatorID] + uptimes[i];
}

Expand Down Expand Up @@ -974,40 +964,31 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version {
/// Create a new validator.
function _createValidator(address auth, bytes calldata pubkey) internal {
uint256 validatorID = ++lastValidatorID;
_rawCreateValidator(auth, validatorID, pubkey, OK_STATUS, currentEpoch(), _now(), 0, 0);
_rawCreateValidator(auth, validatorID, pubkey, currentEpoch(), _now());
}

/// Create a new validator without incrementing lastValidatorID.
function _rawCreateValidator(
address auth,
uint256 validatorID,
bytes calldata pubkey,
uint256 status,
uint256 createdEpoch,
uint256 createdTime,
uint256 deactivatedEpoch,
uint256 deactivatedTime
uint256 createdTime
) internal {
if (getValidatorID[auth] != 0) {
revert ValidatorExists();
}
getValidatorID[auth] = validatorID;
getValidator[validatorID].status = status;
getValidator[validatorID].status = OK_STATUS;
getValidator[validatorID].createdEpoch = createdEpoch;
getValidator[validatorID].createdTime = createdTime;
getValidator[validatorID].deactivatedTime = deactivatedTime;
getValidator[validatorID].deactivatedEpoch = deactivatedEpoch;
getValidator[validatorID].deactivatedTime = 0;
getValidator[validatorID].deactivatedEpoch = 0;
getValidator[validatorID].auth = auth;
getValidatorPubkey[validatorID] = pubkey;
pubkeyAddressToValidatorID[_pubkeyToAddress(pubkey)] = validatorID;

emit CreatedValidator(validatorID, auth, createdEpoch, createdTime);
if (deactivatedEpoch != 0) {
emit DeactivatedValidator(validatorID, deactivatedEpoch, deactivatedTime);
}
if (status != 0) {
emit ChangedValidatorStatus(validatorID, status);
}
}

/// Calculate raw validator epoch transaction reward.
Expand Down
4 changes: 2 additions & 2 deletions contracts/test/UnitTestSFC.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ contract UnitTestSFC is SFC {
return time;
}

function isNode(address addr) internal view override returns (bool) {
function _isNodeDriverAuth(address addr) internal view override returns (bool) {
if (allowedNonNodeCalls) {
return true;
}
return SFC.isNode(addr);
return SFC._isNodeDriverAuth(addr);
}
}

Expand Down
Loading