WIP: pigeon feed upgrade #30
Conversation
|
Copying your statement from Discord:
|
| @@ -219,3 +253,40 @@ def deploy_erc20(_paloma_denom: String[64], _name: String[64], _symbol: String[3 | |||
| event_id: uint256 = unsafe_add(self.last_event_id, 1) | |||
| self.last_event_id = event_id | |||
| log ERC20DeployedEvent(_paloma_denom, erc20, _name, _symbol, _decimals, event_id) | |||
|
|
|||
| @external | |||
| def update_treasury(consensus: Consensus, _feeset_id: uint256, _community_fee: uint256, _security_fee: uint256, _community_wallet: address, _security_wallet: address): | |||
There was a problem hiding this comment.
-
At the moment, the wallet addresses are not part of the governance. We could add them, but would it not be easier to have them be part of compass directly? Or would that be too expensive?
-
What do we do when deploying a new compass. Would it be enough to call
update_treasuryagain to the new compass address?
There was a problem hiding this comment.
If we don't need to update it, we can set them as constant and it will save so much gas. And we can set them on deployment.
There was a problem hiding this comment.
per Discord conversation here, this can stay in governance https://discord.com/channels/987730379182051408/1098323494636372129/1220751522032386149
There was a problem hiding this comment.
We will need to add them to governance then. We'll also need to setup proper handover in case of a new compass (just like for ERC20 tokens)
contracts/Compass.vy
Outdated
| def deposit(paloma_address: bytes32, amount: uint256): | ||
| if msg.value > amount: | ||
| send(msg.sender, unsafe_sub(msg.value, amount)) | ||
| else: | ||
| assert msg.value == amount, "Insufficient deposit" | ||
| _fee: uint256 = unsafe_div(amount * self.security_fee, DENOMINATOR) | ||
| send(self.security_wallet, _fee) | ||
| relayer_fee: uint256 = amount - _fee | ||
| _fee = unsafe_div(amount * self.community_fee, DENOMINATOR) | ||
| send(self.community_wallet, _fee) | ||
| relayer_fee -= _fee | ||
| _nonce: uint256 = self.deposit_nonce | ||
| self.fee_balance[paloma_address] = unsafe_add(self.fee_balance[paloma_address], relayer_fee) | ||
| log Deposited(paloma_address, msg.sender, _nonce, relayer_fee) | ||
| self.deposit_nonce = unsafe_add(_nonce, 1) |
There was a problem hiding this comment.
I think the idea to just move funds to community and security wallet once during deposit is great. At the moment, those fees are defined as percentages of the relayer_fee, which is unique to each relayer, meaning a more expensive relayer would generate more community/security fee, whereas a cheaper relayer would generate less of it.
It also means that more expensive messages would generate more community and security fees, while cheap messages would generate less.
I honestly don't know the reasoning behind that decision, it was made before my time with Volume and I cannot find any background on this. @taariq @verabehr might know more. See https://www.notion.so/volumefi/EPIC-09-26-22-Paloma-Gas-Management-Pigeon-Feed-78319280833e4c7c8ffad368eddb1d91?pvs=4#659fbe58c4f84d56bce0f86afb35839e for background.
Moving the deduction of those fees to the deposit stage changes how they should be defined, and how we as a team should understand and communicate them.
There was a problem hiding this comment.
This is the most cheap solution for gas. Paying fees in SLC will increase SLC gas cost more. SLC gas cost is already too expensive.
There was a problem hiding this comment.
@byte-bandit @webelf101 what is the decision that needs to be made here?
There was a problem hiding this comment.
@taariq my understanding is that
EITHER, we compass becomes more gas expensive and we keep the community and security as percentage of the relayer fee
OR community and security are paid on deposit, but then they would be independent of the actual relayer fee and would need to be a percentage of the deposit amount
| gas_fee: uint256 = gas_estimation * tx.gasprice | ||
| assert _fee_balance >= gas_fee, "Insufficient gas deposit" | ||
| self.fee_balance[paloma_address] = unsafe_sub(_fee_balance, gas_fee) | ||
| send(msg.sender, gas_fee) |
There was a problem hiding this comment.
The current design calls for relayers to actively request a refund from compass, probably to reduce network fees and so they can time when to cash out?
So compass would have to store this funds as escrow for a while until relayers actively request a refund.
There was a problem hiding this comment.
This is to save gas. Storing some data in a contract requires most gas. So I want to avoid storing the relayer fee amount for relayers but send it to the relayer directly.
There was a problem hiding this comment.
further discussed on discord, this is fine as is https://discord.com/channels/987730379182051408/1098323494636372129/1220739514566447134
There was a problem hiding this comment.
Okay, so no claim system, the relayer is reimbursed directly.
How do we protect this against front running? If I see this tx in the mempool and I see that the sender is receiving a nice reward, wouldn't this be a prime candidate for exploiting?
There was a problem hiding this comment.
We simply send ETH but we don't use exchanges. So there can't be any exploit. @byte-bandit
There was a problem hiding this comment.
What's your idea about the "front running"? I might miss something. @byte-bandit
There was a problem hiding this comment.
|
@byte-bandit @webelf101 why is this still open and what do we need to do to close and merge? |
|
This needs changes. I'll be adding them once I get around to it. |
Related Github tickets
Background
A short paragraph on test what exactly this PR solves.
Testing completed