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

Jump to bottom

TACo application refinements #145

Merged
merged 8 commits into from
Oct 27, 2023
Merged

TACo application refinements #145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bfdacd0
TACoApplication: disallow to push reward when no active staking provi…
vzotova Oct 4, 2023
eef00b2
TACoApplication: increase precision for reward calculations
vzotova Oct 6, 2023
f04b3b4
TACoApplication: better state cleaning in case of decreasing authoriz…
vzotova Oct 10, 2023
326dd53
TACoChildApplication: check minimum authorization before confirmation
vzotova Oct 11, 2023
1f22f06
Additional protection for `PolygonChild.setFxRootTunnel` and `Polygon…
vzotova Oct 11, 2023
ac23df5
TACoChildApplication: check null operator during update
vzotova Oct 11, 2023
f7d3972
TACoApplication: setting child app is mandatory
vzotova Oct 12, 2023
073062f
TACoApplication: `public` -> `external` for several view functions
vzotova Oct 12, 2023
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
75 changes: 40 additions & 35 deletions contracts/contracts/TACoApplication.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import "@openzeppelin/contracts/utils/math/Math.sol";
import "@openzeppelin/contracts/utils/math/SafeCast.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/utils/Address.sol";
import "@openzeppelin-upgradeable/contracts/access/OwnableUpgradeable.sol";
import "@threshold/contracts/staking/IApplication.sol";
import "@threshold/contracts/staking/IStaking.sol";
Expand Down Expand Up @@ -143,10 +144,13 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
uint96 deauthorizing; // TODO real usage only in getActiveStakingProviders, maybe remove?
uint64 endDeauthorization;
uint96 tReward;
uint96 rewardPerTokenPaid;
uint160 rewardPerTokenPaid;
uint64 endCommitment;
}

uint256 public constant REWARD_PER_TOKEN_MULTIPLIER = 10 ** 4;
Copy link
Member

Choose a reason for hiding this comment

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

What does this constant represent? What's the multiplier? Is this something for precision?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, name is not perfect. Thing is we have variable called rewardPerToken. With increase of precision now it's actually not per token but per 10000 tokens (that multiplier).

Copy link
Member

Choose a reason for hiding this comment

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

REWARD_CALCULATION_MULTIPLIER?

Copy link
Member Author

@vzotova vzotova Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, this name suits better for FLOATING_POINT_DIVISOR

uint256 internal constant FLOATING_POINT_DIVISOR = REWARD_PER_TOKEN_MULTIPLIER * 10 ** 18;

uint96 public immutable minimumAuthorization;
uint256 public immutable minOperatorSeconds;
uint256 public immutable rewardDuration;
Expand All @@ -171,7 +175,7 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
uint256 public periodFinish;
uint256 public rewardRateDecimals;
uint256 public lastUpdateTime;
uint96 public rewardPerTokenStored;
uint160 public rewardPerTokenStored;
uint96 public authorizedOverall;

/**
Expand All @@ -193,14 +197,22 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
uint256 _deauthorizationDuration,
uint64[] memory _commitmentDurationOptions
) {
uint256 totalSupply = _token.totalSupply();
require(
_rewardDuration != 0 &&
_tStaking.authorizedStake(address(this), address(this)) == 0 &&
_token.totalSupply() > 0 &&
totalSupply > 0 &&
_commitmentDurationOptions.length >= 1 &&
_commitmentDurationOptions.length <= 4,
"Wrong input parameters"
);
// This require is only to check potential overflow for 10% reward
require(
(totalSupply / 10) * FLOATING_POINT_DIVISOR <= type(uint160).max &&
_minimumAuthorization >= 10 ** 18 &&
_rewardDuration >= 1 days,
"Potential overflow"
);
rewardDuration = _rewardDuration;
deauthorizationDuration = _deauthorizationDuration;
minimumAuthorization = _minimumAuthorization;
Expand Down Expand Up @@ -259,10 +271,8 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
* @notice Set contract for multi-chain interactions
*/
function setChildApplication(ITACoRootToChild _childApplication) external onlyOwner {
require(
address(_childApplication) != address(childApplication),
"New address must not be equal to the current one"
);
require(address(childApplication) == address(0), "Child application is already set");
require(Address.isContract(address(_childApplication)), "Child app must be contract");
childApplication = _childApplication;
}

Expand Down Expand Up @@ -309,16 +319,16 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
}

/**
* @notice Returns current value of reward per token
* @notice Returns current value of reward per token * multiplier
*/
function rewardPerToken() public view returns (uint96) {
function rewardPerToken() public view returns (uint160) {
if (authorizedOverall == 0) {
return rewardPerTokenStored;
}
uint256 result = rewardPerTokenStored +
((lastTimeRewardApplicable() - lastUpdateTime) * rewardRateDecimals) /
authorizedOverall;
return result.toUint96();
return result.toUint160();
}

/**
Expand All @@ -331,7 +341,7 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
return info.tReward;
}
uint256 result = (uint256(info.authorized) * (rewardPerToken() - info.rewardPerTokenPaid)) /
1e18 +
FLOATING_POINT_DIVISOR +
info.tReward;
return result.toUint96();
}
Expand All @@ -343,12 +353,15 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
function pushReward(uint96 _reward) external updateReward(address(0)) {
require(msg.sender == rewardDistributor, "Only distributor can push rewards");
require(_reward > 0, "Reward must be specified");
require(authorizedOverall > 0, "No active staking providers");
if (block.timestamp >= periodFinish) {
rewardRateDecimals = (uint256(_reward) * 1e18) / rewardDuration;
rewardRateDecimals = (uint256(_reward) * FLOATING_POINT_DIVISOR) / rewardDuration;
} else {
uint256 remaining = periodFinish - block.timestamp;
uint256 leftover = remaining * rewardRateDecimals;
rewardRateDecimals = (uint256(_reward) * 1e18 + leftover) / rewardDuration;
rewardRateDecimals =
(uint256(_reward) * FLOATING_POINT_DIVISOR + leftover) /
rewardDuration;
}
lastUpdateTime = block.timestamp;
periodFinish = block.timestamp + rewardDuration;
Expand Down Expand Up @@ -443,8 +456,6 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
emit AuthorizationInvoluntaryDecreased(_stakingProvider, _fromAmount, _toAmount);

if (info.authorized == 0) {
_stakingProviderFromOperator[info.operator] = address(0);
info.operator = address(0);
_releaseOperator(_stakingProvider);
}
_updateAuthorization(_stakingProvider, info);
Expand Down Expand Up @@ -506,11 +517,8 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
info.authorized = toAmount;
info.deauthorizing = 0;
info.endDeauthorization = 0;
info.endCommitment = 0;

if (info.authorized == 0) {
_stakingProviderFromOperator[info.operator] = address(0);
info.operator = address(0);
_releaseOperator(_stakingProvider);
}
_updateAuthorization(_stakingProvider, info);
Expand Down Expand Up @@ -538,8 +546,6 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
}

if (info.authorized == 0) {
_stakingProviderFromOperator[info.operator] = address(0);
info.operator = address(0);
_releaseOperator(_stakingProvider);
}
_updateAuthorization(_stakingProvider, info);
Expand Down Expand Up @@ -573,7 +579,7 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
/**
* @notice Returns staking provider for specified operator
*/
function stakingProviderFromOperator(address _operator) public view returns (address) {
function stakingProviderFromOperator(address _operator) external view returns (address) {
return _stakingProviderFromOperator[_operator];
}

Expand All @@ -582,14 +588,14 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
*/
function getOperatorFromStakingProvider(
address _stakingProvider
) public view returns (address) {
) external view returns (address) {
return stakingProviderInfo[_stakingProvider].operator;
}

/**
* @notice Get all tokens delegated to the staking provider
*/
function authorizedStake(address _stakingProvider) public view returns (uint96) {
function authorizedStake(address _stakingProvider) external view returns (uint96) {
return stakingProviderInfo[_stakingProvider].authorized;
}

Expand Down Expand Up @@ -729,9 +735,7 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
emit OperatorBonded(_stakingProvider, _operator, previousOperator, block.timestamp);

info.operatorConfirmed = false;
if (address(childApplication) != address(0)) {
childApplication.updateOperator(_stakingProvider, _operator);
}
childApplication.updateOperator(_stakingProvider, _operator);
}

/**
Expand Down Expand Up @@ -764,10 +768,13 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
* @notice Resets operator confirmation
*/
function _releaseOperator(address _stakingProvider) internal {
stakingProviderInfo[_stakingProvider].operatorConfirmed = false;
if (address(childApplication) != address(0)) {
childApplication.updateOperator(_stakingProvider, address(0));
}
StakingProviderInfo storage info = stakingProviderInfo[_stakingProvider];
_stakingProviderFromOperator[info.operator] = address(0);
info.operator = address(0);
info.operatorConfirmed = false;
info.endDeauthorization = 0;
info.endCommitment = 0;
childApplication.updateOperator(_stakingProvider, address(0));
}

/**
Expand All @@ -777,11 +784,9 @@ contract TACoApplication is IApplication, ITACoChildToRoot, OwnableUpgradeable {
address _stakingProvider,
StakingProviderInfo storage _info
) internal {
if (address(childApplication) != address(0)) {
// TODO send both authorized and eligible amounts in case of slashing from child app
uint96 eligibleAmount = getEligibleAmount(_info);
childApplication.updateAuthorization(_stakingProvider, eligibleAmount);
}
// TODO send both authorized and eligible amounts in case of slashing from child app
uint96 eligibleAmount = getEligibleAmount(_info);
childApplication.updateAuthorization(_stakingProvider, eligibleAmount);
}

//-------------------------Slashing-------------------------
Expand Down
36 changes: 25 additions & 11 deletions contracts/contracts/coordination/TACoChildApplication.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ contract TACoChildApplication is ITACoRootToChild, ITACoChildApplication, Initia
ITACoChildToRoot public immutable rootApplication;
address public coordinator;

uint96 public immutable minimumAuthorization;

mapping(address => StakingProviderInfo) public stakingProviderInfo;
mapping(address => address) public stakingProviderFromOperator;

Expand All @@ -35,12 +37,14 @@ contract TACoChildApplication is ITACoRootToChild, ITACoChildApplication, Initia
_;
}

constructor(ITACoChildToRoot _rootApplication) {
constructor(ITACoChildToRoot _rootApplication, uint96 _minimumAuthorization) {
require(
address(_rootApplication) != address(0),
"Address for root application must be specified"
);
require(_minimumAuthorization > 0, "Minimum authorization must be specified");
rootApplication = _rootApplication;
minimumAuthorization = _minimumAuthorization;
_disableInitializers();
}

Expand Down Expand Up @@ -79,16 +83,20 @@ contract TACoChildApplication is ITACoRootToChild, ITACoChildApplication, Initia
StakingProviderInfo storage info = stakingProviderInfo[stakingProvider];
address oldOperator = info.operator;

if (stakingProvider != address(0) && operator != oldOperator) {
info.operator = operator;
// Update operator to provider mapping
stakingProviderFromOperator[oldOperator] = address(0);
stakingProviderFromOperator[operator] = stakingProvider;
info.operatorConfirmed = false;
// TODO placeholder to notify Coordinator
if (stakingProvider == address(0) || operator == oldOperator) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if it's a dumb question, but why not manage this with a require statement?

Suggested change
if (stakingProvider == address(0) || operator == oldOperator) {
require (stakingProvider != address(0) && operator != oldOperator);

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not dumb question. Because there is no real point to fail in xchain txs, it will be just skipped and that's it. But some sanity check to decrease gas consumption we want to do

Copy link
Member

Choose a reason for hiding this comment

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

got it, thanks!

return;
}

emit OperatorUpdated(stakingProvider, operator);
info.operator = operator;
// Update operator to provider mapping
stakingProviderFromOperator[oldOperator] = address(0);
if (operator != address(0)) {
stakingProviderFromOperator[operator] = stakingProvider;
}
info.operatorConfirmed = false;
// TODO placeholder to notify Coordinator

emit OperatorUpdated(stakingProvider, operator);
}

function _updateAuthorization(address stakingProvider, uint96 amount) internal {
Expand All @@ -105,7 +113,10 @@ contract TACoChildApplication is ITACoRootToChild, ITACoChildApplication, Initia
require(msg.sender == coordinator, "Only Coordinator allowed to confirm operator");
address stakingProvider = stakingProviderFromOperator[_operator];
StakingProviderInfo storage info = stakingProviderInfo[stakingProvider];
require(info.authorized > 0, "No stake associated with the operator");
require(
info.authorized >= minimumAuthorization,
"Authorization must be greater than minimum"
);
// TODO maybe allow second confirmation, just do not send root call?
require(!info.operatorConfirmed, "Can't confirm same operator twice");
info.operatorConfirmed = true;
Expand All @@ -117,7 +128,10 @@ contract TACoChildApplication is ITACoRootToChild, ITACoChildApplication, Initia
contract TestnetTACoChildApplication is AccessControlUpgradeable, TACoChildApplication {
bytes32 public constant UPDATE_ROLE = keccak256("UPDATE_ROLE");

constructor(ITACoChildToRoot _rootApplication) TACoChildApplication(_rootApplication) {}
constructor(
ITACoChildToRoot _rootApplication,
uint96 _minimumAuthorization
) TACoChildApplication(_rootApplication, _minimumAuthorization) {}

function initialize(address _coordinator, address[] memory updaters) external initializer {
coordinator = _coordinator;
Expand Down
5 changes: 4 additions & 1 deletion contracts/contracts/testnet/LynxSet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ contract MockPolygonChild is Ownable, ITACoChildToRoot, ITACoRootToChild {
}

contract LynxTACoChildApplication is TACoChildApplication, Ownable {
constructor(ITACoChildToRoot _rootApplication) TACoChildApplication(_rootApplication) {}
constructor(
ITACoChildToRoot _rootApplication,
uint96 _minimumAuthorization
) TACoChildApplication(_rootApplication, _minimumAuthorization) {}

function setCoordinator(address _coordinator) external onlyOwner {
require(_coordinator != address(0), "Coordinator must be specified");
Expand Down
5 changes: 4 additions & 1 deletion contracts/contracts/testnet/TapirSet.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "../coordination/TACoChildApplication.sol";

contract TapirTACoChildApplication is TACoChildApplication, Ownable {
constructor(ITACoChildToRoot _rootApplication) TACoChildApplication(_rootApplication) {}
constructor(
ITACoChildToRoot _rootApplication,
uint96 _minimumAuthorization
) TACoChildApplication(_rootApplication, _minimumAuthorization) {}

function setCoordinator(address _coordinator) external onlyOwner {
require(_coordinator != address(0), "Coordinator must be specified");
Expand Down
5 changes: 5 additions & 0 deletions contracts/xchain/PolygonChild.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ contract PolygonChild is FxBaseChildTunnel, Ownable {
require(success, "Child tx failed");
}

function setFxRootTunnel(address _fxRootTunnel) external override onlyOwner {
require(fxRootTunnel == address(0x0), "FxBaseChildTunnel: ROOT_TUNNEL_ALREADY_SET");
fxRootTunnel = _fxRootTunnel;
}

function setChildApplication(address _childApplication) public onlyOwner {
childApplication = _childApplication;
}
Expand Down
5 changes: 4 additions & 1 deletion contracts/xchain/PolygonRoot.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ contract PolygonRoot is FxBaseRootTunnel {
address _rootApplication,
address _fxChildTunnel
) FxBaseRootTunnel(_checkpointManager, _fxRoot) {
require(_rootApplication != address(0), "Wrong input parameters");
require(
_rootApplication != address(0) && _fxChildTunnel != address(0),
"Wrong input parameters"
);
rootApplication = _rootApplication;
fxChildTunnel = _fxChildTunnel;
}
Expand Down
45 changes: 23 additions & 22 deletions deployment/constructor_params/lynx/child.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,28 @@ deployment:
chain_id: 80001

artifacts:
dir: ./deployment/artifacts/
filename: lynx.json
dir: ./deployment/artifacts/
filename: lynx.json

contracts:
- MockPolygonChild
- ProxyAdmin
- LynxTACoChildApplication:
_rootApplication: $MockPolygonChild
- TransparentUpgradeableProxy:
_logic: $LynxTACoChildApplication
admin_: $ProxyAdmin
_data: $bytes:0x
- LynxRitualToken:
_totalSupplyOfTokens: 10000000000000000000000000
- Coordinator:
_application: $TransparentUpgradeableProxy:LynxTACoChildApplication
_timeout: 3600
_maxDkgSize: 4
_admin: $deployer
_currency: $LynxRitualToken
_feeRatePerSecond: 1
- GlobalAllowList:
_coordinator: $Coordinator
_admin: $deployer
- MockPolygonChild
- ProxyAdmin
- LynxTACoChildApplication:
_rootApplication: $MockPolygonChild
_minimumAuthorization: 40000000000000000000000
Copy link
Member

Choose a reason for hiding this comment

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

Something to think about here is that the value used in root.yml for the root application would need to match the value used here... 🤔 (cc @KPrasch )

Copy link
Member

Choose a reason for hiding this comment

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

That's why we need a separate constants section or file

Copy link
Member

@derekpierre derekpierre Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed. Now that it is a yml file, perhaps we can specify a dependent constants.yml file ... that allows for a dictionary of variables/values to be specified. I'm less familiar with yml spec.

- TransparentUpgradeableProxy:
_logic: $LynxTACoChildApplication
admin_: $ProxyAdmin
_data: $bytes:0x
- LynxRitualToken:
_totalSupplyOfTokens: 10000000000000000000000000
- Coordinator:
_application: $TransparentUpgradeableProxy:LynxTACoChildApplication
_timeout: 3600
_maxDkgSize: 4
_admin: $deployer
_currency: $LynxRitualToken
_feeRatePerSecond: 1
- GlobalAllowList:
_coordinator: $Coordinator
_admin: $deployer
Loading
Loading