Fix bug in DeployRaffle script causing insufficient balance when testing with VRFCoordinatorV2_5Mock

[sefkoleen]

Removed the IF statement inside of the DeployRaffle.s.sol, which calls the create and fund subscription only if the subId == 0. The problem was that the subId is never 0, because the subId is created inside the HelperConfig.s.sol, which in the DeployRaffle contract is called before this IF statement. This introduced a bug which when testing the testFulFillRandomWordsPicksAWinnerResetsAndSendsMoney() from the RaffleTest.t.sol always fails with InsufficientBalance() error. This is because the fundSubscription() is never called, and the subscription never has link to execute fulfillRandomWords(). I tested this for a few days and logged the prevBal (which was always 0) and payment from the VRFCoordinatorV2_5Mock.sol, which confirmed this and led me to solving this bug in the end.

[PatrickAlphaC]

Hmm... I'm not sure this quite makes sense. I have just pushed an update where we actually update the sub id.

We don't want to always make a subid, because there are scenarios where a user already has one and they don't want to spend the extra gas making another one!

I have rerun the tests on my new push, can you do a pull and see if the tests are working for you now?

If not, can you please give me:

1. The exact command you're running
2. The exact error you're running into from that push?

[sefkoleen]

I am still trying to figure out why I removed the IF statement instead of attacking the real problem, and I am sorry if I make mistakes, I am just a student of yours and am really passionate about web3 development, which is why I wanna contribute as much as I can in the future.

The problem I had is directly connected the the FundSubscritpion in the Deploy Script. Each time when testing using Anvil, the first thing that happens is that the HelperConfig creates a new Subscription for us, which is not yet funded.

The next step of the Deploy Script is to check IF the subId is 0, and ONLY if this is the case we are calling the FundSubscription to fund our Subscription. Since our subId is not 0, because HelperConfig created a new Subscription for us before this check happened, our Subscription is not being funded at all. The reason is that the only place we are calling FundSubscription is inside of the IF statement block.

So what I should have said in the first pull request is that we should probably keep the IF statement block to create the Subscription if the subId is 0, but to add another check if we are on Anvil, it should just call FundSubscription. This way If we are not on Anvil, the check with the subId == 0 still makes sense and is still keeps users who have a subId away from spending more gas than they should.

if (block.chainid == LOCAL_CHAIN_ID) {
  FundSubscription fundSubscription = new FundSubscription();
  fundSubscription.fundSubscription(config.vrfCoordinatorV2_5, config.subscriptionId, config.link, config.account);
} else if (subId == 0) {
  CreateSubscription createSubscription = new CreateSubscription();
  (config.subscriptionId, config.vrfCoordinatorV2_5) = createSubscription.createSubscription(config.vrfCoordinatorV2_5, config.account);
  FundSubscription fundSubscription = new FundSubscription();
  fundSubscription.fundSubscription(config.vrfCoordinatorV2_5, config.subscriptionId, config.link, config.account);
}

I checked the code before explaining and I hope that I did not miss something, and to make this even more clear (open for a negative reply or critique), I will write the process order as I see it:

1. HelperConfig Contract Creation (subscriptionId Created)
2. AddConsumer Contract Creation
3. Storing NetworkConfig into variable
4. Calling FundSubscription ONLY if the subId == 0 (which it never is when doing tests with Anvil, which is why my tests failed with InsufficientBalance, because my Subscriptions were never funded)
5. Deploying Raffle Contract
6. Adding the Consumer

[cromewar]

Hello @sefkoleen, What a nice topic to discuss! You are right that the subscription will work differently on Anvil as we are not technically accessing a VRF node. The implementation you suggest is clever, but I would take something into consideration.

Of course, you can add a filter to understand when you are on a live network and when you are on your local anvil network. However, when working on big-scale projects, you will notice you will have two types of tests (actually more, but that's another topic): unit tests and integration tests.

Unit test: will run exclusively on your local environment, and when using using external services like Chainlink VRF you will always use Mocks. Integration test: This one you will test to local/pre-deployment scenarios, like a Dev-branch on a web2 project or in our case Sepolia.

So, even though the suggestion is correct, It might not be suitable for a project schema. as both type of test will be run separately anyway.

With that said, I have two things to say:

• You are doing an amazing job, and I would like you to submit another pr.
• Are you up to a user interview? we are interviewing users to improve the Updratt experience, your help is invaluable for us.

[sefkoleen]

Hi @cromewar. I would like to thank you for the in depth explanation.

After reading what you wrote, I went back to the unit test where I saw that, there is a if block which checks if we are on the local chain and funds our mock coordinator with some link in that case. This made a lot of sense, after you explained that unit tests are exclusively meant for our local environment, so we do not need to clutter any other part of the code which is not directly connected to this.

Regarding the two things:

• I am 100% going to continue contributing as much as I can.
• Definitely interested to do an interview, let's just match our time-zones, and decide on a good time for both of us.

[cromewar]

Can you drop me a follow on X (Twitter) at @cromewar? , @sefkoleen so I can send you a Calendly invite. Or let's find another way to communicate; I don't want you to drop an email address here as this is a public repo.

[sefkoleen]

Sure, just followed you. It's @flownect on X.