-
Notifications
You must be signed in to change notification settings - Fork 286
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
[feat] collect txns fees to a specific account #4265
Conversation
looks good. How is this change tested? We need to at least test on localnet and testnet before launching on mainnet since it's token-related critical change. |
@here, my ask here is to have a new test written in https://github.com/harmony-one/harmony-test so this feature can be tested at every new PR and be part of our CI/CD thanks. |
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.
Looks good. Please add a test as Soph described; let me know if you need help.
because testnet has not been fixed, we can only test it in localnet. will post test log later. |
test js script: // fee.js
const ethers = require('ethers')
const provider = new ethers.providers.JsonRpcProvider('http://127.0.0.1:9500')
const feeCollector = '0x19E7E376E7C213B7E7e7e46cc70A5dD086DAff2A'
const faucetPrivateKey = ''
const faucetWallet = new ethers.Wallet(faucetPrivateKey, provider)
async function main() {
const getBalance = async ()=>({
feeCollector: await provider.getBalance(feeCollector).then(ethers.utils.formatEther),
faucet: await faucetWallet.getBalance().then(ethers.utils.formatEther)
})
const beforeBalance = await getBalance()
const gasPrice = ethers.utils.parseEther('0.001') // 21000 * 0.001 ether = 21 ether
await faucetWallet.sendTransaction({to:faucetWallet.address, gasPrice}).then(tx=>tx.wait())
const afterBalance = await getBalance()
console.log('beforeBalance:', beforeBalance)
console.log('afterBalance :', afterBalance)
}
main() run log: beforeBalance: { feeCollector: '0.0', faucet: '10000000000.0' }
afterBalance : { feeCollector: '21.0'', faucet: '9999999979.0' } |
Can you move the checks here to be inside harmony/internal/params/config.go Lines 706 to 715 in 7c1c43e
Secondly, can you ensure the |
fixed |
@peekpi will that work for shard 1/2/3 fees ? cc @MaxMustermann2 Also let's assume one day we need a 5th shard, will that still work ? here are some test case scenario : What about future cross shard contract transaction fee ? |
I can confirm that this will work. |
fee are always collected in src shard. for future cross shard contract transaction, we need to handle this scenario carefully. fee collection takes place in function |
If the |
does pr #4165 only support cross shard transfer? or also support cross shard contract call? |
It supports only transfers. And it's actually disabled for smart contract (although that needs some more hardening in my opinion). Please ignore my original comment then. |
b58aaa1
to
253f7f1
Compare
a88fc73
to
6524a6a
Compare
epoch activation for devnet at epoch 574
Issue
#4261
After FeeCollectEpoch, all the transactions fees will be collected into the feeCollector account instead of be burned.
If the feeCollector account is a contract, it will not trigger any of its functions(fallback()/receive() in solidity).
feeCollector’s balance increases after each transaction is executed.