210 test if account has enough balance to request service #228

KelvinThai · 2023-02-06T09:21:07Z

No description provided.

… test case

martinkersner

I think we have to check the required balance in a different way. You can see more details in comments.

martinkersner · 2023-02-06T09:50:09Z

contracts/src/v0.1/RequestResponseCoordinator.sol

+
+        uint256 payment;
+
+        payment = calculatePaymentAmount(


In my opinion the payment value which you compute is something completely different than we are interested in. Similar computation is done inside of fulfill* functions but the purpose and final value will be very different.

What I was thinking to do it is to set some predefined required balance that has to be in account, otherwise the request will be rejected.

oh, then that makes the problem much simpler. the way I'm doing is to utilize the payment calculation in the fullfillment, the only difference is the gas at the time of fullfill and the time at request, but that is a very small number.
In your case, we can just simply implement a global MinBalance viriable and check that in every request, am I getting it right?

I think the gas difference will be quite significant, but even if it is small difference it does not compute the correct value, therefore we should not use it. It could lead to failed fulfillments.

We can define minBalance which would be part of Prepayment configuration. It will be simpler solution and everybody will understand that it is predefined. The value has to be selected with care because it can have similar problems like the current solution.

Agreed. In case we implement the minBalance, it should be implemented in the Coordinators, not Prepayment because the price is different for every service.

Yes, you are right!

How about I refactor this with minBalance and setter function for this? It would raise centralize issues, but do you think it's good enough for now?

I think it is good for now. We can solve similar centralization problems later on. For now, the goal is to make sure that people with balance lower than minBalance cannot request anything. However, we must keep in mind that at first nobody will pay for the service, so the value would be 0.

Perfect! let's go for it

martinkersner · 2023-02-06T09:50:26Z

contracts/src/v0.1/VRFCoordinator.sol

+
+        uint256 payment;
+
+        payment = calculatePaymentAmount(


The same problem as described above.

martinkersner · 2023-02-06T09:52:53Z

contracts/test/v0.1/Prepayment.test.ts

@@ -161,6 +161,8 @@ describe('Prepayment contract', function () {

    await prepaymentContractConsumerSigner.addConsumer(accId, consumerContract.address)
    await prepaymentContract.addCoordinator(coordinatorContract.address)
+    const value = 1_000_000_000_000_000


Let's prefer to use ethers.utils.parseUnits instead of raw wei values. It will be easier to read.

https://docs.ethers.org/v5/api/utils/display-logic/#utils-parseUnits

There is also already function called parseKlay.

@KelvinThai Can you please update this value?

martinkersner · 2023-02-06T09:56:43Z

contracts/test/v0.1/RequestResponseConsumerContract.test.ts

+
+    await expect(
+      consumerContract.requestData(accId, maxGasLimit, {
+        gasLimit: 500_000


Do we have to specify the gasLimit?

We dont have to, it runs either way, it just my habit to include a high gas limit in test, I some case it doesn't work on local with low gas limit

martinkersner · 2023-02-06T09:59:57Z

contracts/test/v0.1/VRF.test.ts

+
+    await prepaymentContractConsumerSigner.addConsumer(accId, consumerContract.address)
+    await prepaymentContract.addCoordinator(coordinatorContract.address)
+    const value = 1_000_000_000_000


The deposit is not high enough, that's why it should fail?

yes, tha's the purpose of the test

I think it is not exactly obvious from this test that such number is too small. After the implementation is changed to minBalance. I think it will make more sense but the deposited value still should be defined based on minBalance and testing the it is smaller than minBalance.

martinkersner

There are few cosmetic changes mostly related to

where we call setMinBalance
how do we convert klay to peb (let's do to same everywhere please)
suggesting to remove an extra unused variables

martinkersner · 2023-02-07T05:08:45Z

contracts/src/v0.1/RequestResponseCoordinator.sol

    function requestData(
        Orakl.Request memory req,
        uint32 callbackGasLimit,
        uint64 accId
    ) external nonReentrant returns (uint256 requestId) {
+        uint256 startGas = gasleft();


I don't think startGas is used anywhere in this function.

martinkersner · 2023-02-07T05:09:16Z

contracts/src/v0.1/VRFCoordinator.sol

@@ -450,7 +458,14 @@ contract VRFCoordinator is
        uint32 callbackGasLimit,
        uint32 numWords
    ) external nonReentrant onlyValidKeyHash(keyHash) returns (uint256 requestId) {
+        uint256 startGas = gasleft();


I don't think startGas is used anywhere in this function.

martinkersner · 2023-02-07T05:10:10Z

contracts/src/v0.1/VRFCoordinator.sol

@@ -450,7 +458,14 @@ contract VRFCoordinator is
        uint32 callbackGasLimit,
        uint32 numWords
    ) external nonReentrant onlyValidKeyHash(keyHash) returns (uint256 requestId) {
+        uint256 startGas = gasleft();
        bool isDirectPayment = false;


Let's move this closer to the call of requestRandomWordsInternal. You have included the check in between one logical piece of code.

Sorry, I dont understand this Comment, we remove this unused variable?

Github does not show it nicely. The comment is for line 462, but it looks like I am commenting about 461. Let me try to explain it with example.

Before adding the balance check, the code looked like this

bool isDirectPayment = false; requestId = requestRandomWordsInternal( keyHash, accId, callbackGasLimit, numWords, isDirectPayment );

isDirectPayment variable was defined close requestRandomWordsInternal so the meaning of false value is apparent. However, after adding the check for minBalance, it looks as follows

bool isDirectPayment = false; (uint256 balance, , , ) = s_prepayment.getAccount(accId); if (balance < s_minBalance) { revert InsufficientPayment(balance, s_minBalance); } requestId = requestRandomWordsInternal( keyHash, accId, callbackGasLimit, numWords, isDirectPayment );

Definition of isDirectPayment is separated from requestRandomWordsInternal call by check for minBalance.

It might be better to check the comments from files section where it is easier to understand what is being commented on.

martinkersner · 2023-02-07T05:10:31Z

contracts/test/v0.1/Prepayment.test.ts

@@ -161,6 +161,8 @@ describe('Prepayment contract', function () {

    await prepaymentContractConsumerSigner.addConsumer(accId, consumerContract.address)
    await prepaymentContract.addCoordinator(coordinatorContract.address)
+    const value = 1_000_000_000_000_000


@KelvinThai Can you please update this value?

martinkersner · 2023-02-07T05:13:17Z

contracts/test/v0.1/RequestResponseConsumerContract.test.ts

@@ -31,6 +32,9 @@ describe('Request-Response user contract', function () {

    await (await coordinatorContract.registerOracle(rrOracle0)).wait()

+    const minBalance = ethers.utils.parseUnits('1', 15)


I think this is quite hard to read and understand the meaning of this value. How about doing this instead which should produce the same value.

Suggested change

const minBalance = ethers.utils.parseUnits('1', 15)

const minBalance = ethers.utils.parseUnits('0.001')

martinkersner · 2023-02-07T05:14:50Z

contracts/test/v0.1/VRF.test.ts

+      gasAfterPaymentCalculation,
+      Object.values(feeConfig)
+    )
+    const minBalance = ethers.utils.parseUnits('1', 15)


Same as described above, it is hard to read and consequently understand the desired value.

martinkersner · 2023-02-07T05:16:17Z

contracts/test/v0.1/RequestResponseConsumerContract.test.ts

@@ -31,6 +32,9 @@ describe('Request-Response user contract', function () {

    await (await coordinatorContract.registerOracle(rrOracle0)).wait()

+    const minBalance = ethers.utils.parseUnits('1', 15)
+    await coordinatorContract.setMinBalance(minBalance)


I think this should be part of fixture where we define all important settings related to deployment.

yes, let's move it to fixture.

martinkersner · 2023-02-07T05:16:27Z

contracts/test/v0.1/VRF.test.ts

+      Object.values(feeConfig)
+    )
+    const minBalance = ethers.utils.parseUnits('1', 15)
+    await coordinatorContract.setMinBalance(minBalance)


Same as above. I think this should be part of fixture where we define all important settings related to deployment.

bayram98

I have tested running tests locally, all of them as working fine.

LGTM!

martinkersner

LGTM!

KelvinThai added 2 commits February 6, 2023 11:47

check prepayment blance in requestRandomWords, test case

67342c0

check prepayment balance in requestData - requestResponseCoordinator,…

a1e4493

… test case

KelvinThai linked an issue Feb 6, 2023 that may be closed by this pull request

Test if Account has enough balance to request service #210

Closed

KelvinThai requested a review from martinkersner February 6, 2023 09:21

martinkersner requested changes Feb 6, 2023

View reviewed changes

martinkersner assigned KelvinThai Feb 6, 2023

refactored check minBalance vrf, requestResponse, test case

963dfad

KelvinThai requested a review from bayram98 as a code owner February 6, 2023 13:15

KelvinThai requested a review from martinkersner February 7, 2023 02:58

martinkersner requested changes Feb 7, 2023

View reviewed changes

bayram98 approved these changes Feb 7, 2023

View reviewed changes

fixed Minbalance as conmments

c980541

KelvinThai requested a review from martinkersner February 7, 2023 06:30

martinkersner approved these changes Feb 7, 2023

View reviewed changes

martinkersner merged commit f246a1d into master Feb 7, 2023

martinkersner deleted the 210-test-if-account-has-enough-balance-to-request-service branch February 7, 2023 10:40

martinkersner unassigned KelvinThai Feb 9, 2023

		@@ -31,6 +32,9 @@ describe('Request-Response user contract', function () {

		await (await coordinatorContract.registerOracle(rrOracle0)).wait()

		const minBalance = ethers.utils.parseUnits('1', 15)

210 test if account has enough balance to request service #228

210 test if account has enough balance to request service #228

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!