193 burn fee collected in prepayment #199
Conversation
contracts/src/v0.1/Prepayment.sol
Outdated
| @@ -18,10 +18,13 @@ contract Prepayment is | |||
| uint16 public constant MAX_CONSUMERS = 100; | |||
| bytes32 public constant WITHDRAWER_ROLE = keccak256("WITHDRAWER_ROLE"); | |||
| bytes32 public constant COORDINATOR_ROLE = keccak256("COORDINATOR_ROLE"); | |||
| uint16 public constant MIN_BURN_RATIO = 20; | |||
There was a problem hiding this comment.
I think the range should be between 0 and 100, and because such range is quite obvious, we do not need to define global variables for that. If you agree, please remove both MIN_BURN_RATIO and MAX_BURN_RATIO.
There was a problem hiding this comment.
That was my first thought, theoretically, we could set it to 100% and burn all the fee. that would 8000 not ideal for our future operator, also the auditor would mark that as a "centralized issue". So I thought It would be better to make the decision at the beginning and make it constant
There was a problem hiding this comment.
I think we have to be flexible. We should not set some rules before getting approval from KF side. Keeping it in between 0 and 100 is the best we can do in my opinion.
There was a problem hiding this comment.
Agreed. flexibility and decentralization are always 2 opposite metrics of blockchain, sometimes we sacrifice one for the other.
How about we keep the const but set that to 0 --> 100 for now? Then we inform KF that all the centralized issues may arise if we go that path, then we can decide on a suitable boundary for burning rates.
The reason I want to keep those constant is to be totally transparent for our 3rd party node operators.
There was a problem hiding this comment.
Can you please update the code according to your suggestion?
Co-authored-by: Martin Kersner <martin@bisonai.com>
Co-authored-by: Martin Kersner <martin@bisonai.com>
| MAX_REQUEST_CONFIRMATIONS | ||
| ); | ||
| } | ||
| // if (minimumRequestConfirmations > MAX_REQUEST_CONFIRMATIONS) { |
There was a problem hiding this comment.
Please don't keep commented code. We don't plan to use it, so it is better to remove it.
| s_config.minimumRequestConfirmations, | ||
| MAX_REQUEST_CONFIRMATIONS | ||
| ); | ||
| if (requestConfirmations > MAX_REQUEST_CONFIRMATIONS) { |
There was a problem hiding this comment.
This has to be removed as well. We are removing requestConfirmations from everywhere.
| await prepaymentContract.connect(node).chargeFee(accId, feeAmount, node.address) | ||
| ).wait() | ||
| const txEvent = prepaymentContract.interface.parseLog(txReceipt.events[0]) | ||
| const { acc, oldBalance, newBalance, burnAmount } = txEvent.args |
There was a problem hiding this comment.
acc, oldBalance and newBalance are not used, we do not have to extract them from event.
| const amount = burnAmount.toNumber() + balanceNode.toNumber() | ||
|
|
||
| expect(feeAmount).to.be.equal(amount) | ||
| console.log('burn amount', burnAmount.toString(), '- node balance', balanceNode.toString()) |
There was a problem hiding this comment.
Let's not include logs to tests. We can use them for debugging, but otherwise we are just interested if tests passes or fail.
|
|
||
| it('Should chargeFee with burn token', async function () { | ||
| const { prepaymentContract, deployer, accId } = await loadFixture(deployFixture) | ||
| const accounts = await ethers.getSigners() |
There was a problem hiding this comment.
It would be better to use named accounts rather than using getSigners directly. We have already defined quite many of them at https://github.com/Bisonai-CIC/orakl/blob/2cd73a4a7243188a21bd279048ecf0eb79e7eba8/contracts/hardhat.config.ts#L48-L74
|
Yes, I noticed the offchain part as well, we will working on it in this branch |
| @@ -1,3 +1,2 @@ | |||
| registry=https://registry.yarnpkg.com/ | |||
| @bisonai-cic:registry=https://npm.pkg.github.com | |||
| //npm.pkg.github.com/:_authToken=${NPM_TOKEN} | |||
There was a problem hiding this comment.
@KelvinThai The problem is here, that's why the test breaks. Please put it back. We need it there.
There was a problem hiding this comment.
LGTM! But please remove the minimumRequestConfirmations also from following files:
- https://github.com/Bisonai-CIC/orakl/blob/193-burn-fee-collected-in-prepayment/contracts/test/v0.1/VRF.config.ts
- https://github.com/Bisonai-CIC/orakl/blob/193-burn-fee-collected-in-prepayment/contracts/config/localhost/vrf.json
- https://github.com/Bisonai-CIC/orakl/blob/193-burn-fee-collected-in-prepayment/contracts/config/baobab/vrf.json
In this PR: