-
Notifications
You must be signed in to change notification settings - Fork 228
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.
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
Add fee manager for erc-to-erc bridge in POSDAO chain #166
Add fee manager for erc-to-erc bridge in POSDAO chain #166
Conversation
…c-erc-posdao-chain # Conflicts: # deploy/src/erc_to_erc/home.js # package-lock.json # package.json
contracts/upgradeable_contracts/erc20_to_erc20/FeeManagerErcToErcPOSDAO.sol
Outdated
Show resolved
Hide resolved
uint256 _foreignDailyLimit, | ||
uint256 _foreignMaxPerTx, | ||
address _owner, | ||
address _blockReward, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in common case the erc20-to-erc20
bridge mode does not require the block reward contract. Reward could be assigned directly by the fee manager. So, I think, we need to have a separate contract like POSDAOHomeBridgeErcToErc
which will extend HomeBridgeErcToErc
and redefine rewardableInitialize()
method with another set of parameters, this contract will also contain blockRewardContract()
and setBlockRewardContract()
.
Another idea is that blockRewardContract
and setBlockRewardContract
should be implemented in the fee manager since they are related to the fee distribution functionality. In this case the bridge contract should proxy calls to the fee manager contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added POSDAOHomeBridgeErcToErc
here 8ab76b9.
Another idea is that blockRewardContract and setBlockRewardContract should be implemented in the fee manager since they are related to the fee distribution functionality. In this case the bridge contract should proxy calls to the fee manager contract.
I implemented this approach, it makes sense to manage blockReward logic on fee manager since it is related to this contract functionality.
contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol
Outdated
Show resolved
Hide resolved
contracts/upgradeable_contracts/erc20_to_erc20/FeeManagerErcToErcPOSDAO.sol
Outdated
Show resolved
Hide resolved
contracts/upgradeable_contracts/erc20_to_erc20/FeeManagerErcToErcPOSDAO.sol
Outdated
Show resolved
Hide resolved
…c-erc-posdao-chain # Conflicts: # contracts/upgradeable_contracts/erc20_to_erc20/HomeBridgeErcToErc.sol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding here a part of the Slack chat:
We have three possible case here
- Non-rewardable: ordinary bridge contract, ordinary validators contract, ordinary token contract
- Rewardable bridge validators: so far it is not implemented yet, so, the corresponding warning could be printed and the script could stop. But in the future it will be: rewardable bridge contract, rewardable validators contract, ordinary token contract, ordinary fee manager (edited)
- Reward for network validators: rewardable contract, ordinary validator contract, rewardable token contract (or corresponding configuration), the fee manager working with the block reward contract
It could be differentiated by the following parameters: HOME_REWARDABLE
(if it is true, consider the cases 2 or 3) and BLOCK_REWARD_ADDRESS
(if it is configured and not zero, consider the case 3).
@akolotov Applied logic on deployment script using |
if (feeManager != address(0)) { | ||
uint256 fee = calculateFee(valueToMint, false, feeManager, FOREIGN_FEE); | ||
distributeFeeFromAffirmation(fee, feeManager); | ||
emit FeeDistributedFromAffirmation(fee, txHash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need these event anymore after #176
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Updated logic here 6b25ffb
(recipient, amount, txHash, contractAddress) = Message.parseMessage(_message); | ||
uint256 fee = calculateFee(amount, true, feeManager, HOME_FEE); | ||
distributeFeeFromSignatures(fee, feeManager); | ||
emit FeeDistributedFromSignatures(fee, txHash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need these event anymore after #176
Closes #160