test: use "expectCall" overload with "count" param

[PaulRBerg]

Addresses #30.

Note: CI is still failing due to test_BatchCancelMultiple. This is another bug for which I will create a separate issue.

[andreivladbrg]

Why not define the count parameter as uint64 in the expect call function parameters? This way, when calling the function with the batch size, the conversion will be handled automatically.

Why not use this function directly in the expect call where the count is 1?

https://github.com/PaulRBerg/prb-test/blob/c56f7a296ab1b2d394d9ad858f5f0f1cdb2969b3/src/Vm.sol#L520

If you choose not to make the change to the function suggested above, I would recommend replacing uint64(1) with just 1.

[PaulRBerg]

Why not define the count parameter as uint64 in the expect call function parameters?

Because we would have to cast all parameters, e.g. defaults.BATCH_SIZE() to uint64, i.e. uint64(defaults.BATCH_SIZE()).

the conversion will be handled automatically

It wouldn't be handled automatically, no.

Why not use this function directly in the expect call where the count is 1?

Because that's the old expectCall and it is unclear how Foundry will handle it in the long-term. They are still scrambling to fix all breaking changes introduced recently, see the convo in foundry-rs/foundry#4845.

I would recommend replacing uint64(1) with just 1.

Huh, not sure why I did not write it like that in the first place! Good idea.

[PaulRBerg]

@andreivladbrg is this good to merge now?

[andreivladbrg]

It wouldn't be handled automatically, no.

RIght, in this case let's declare BATCH_SIZE as uint64 to avoid the conversions.

[PaulRBerg]

If we do that, we will have issues with BATCH_SIZE in other places.

Also, defining the count param as uint64 would pose problems with future uses of these helpers.

Let's stick with uint256.

[PaulRBerg]

@andreivladbrg - good to merge now?

[andreivladbrg]

we will have issues with BATCH_SIZE in other places.

I don't think that's correct, there would be no impact on anything.

defining the count param as uint64 would pose problems with future uses of these helpers.

I cannot think of one. Can you give an example?

Let's stick with uint256.

IMO there is no reason to stick with uint256.

[andreivladbrg]

Created this PR #34 to address it

[PaulRBerg]

It doesn't make sense to define the type of a variable/ constant based on its use in a particular helper function. To make an analogy - you don't change all door locks in your house because your front door needs to have a particular lock for whatever reason.

defining the count param as uint64 would pose problems with future uses of these helpers.

I cannot think of one. Can you give an example?

When fuzzing the batch size, you would have to cast the call count to uint64 if you change the function type.

IMO there is no reason to stick with uint256.

Quite the opposite - no reason to use uint64 when uint256 is the default word size of the EVM. Using uint64 would "poison the well".

[PaulRBerg]

I don't think that's correct, there would be no impact on anything

You're right insofar as Solidity does the typecasting implicitly in this case, but as my previous comment explained, doing so would only give us homework in the long haul.

Again - we don't define the type of a variable based on its use in a particular helper function (or set of helper functions).

[PaulRBerg]

I had a change of heart about this:

Why not use this function directly in the expect call where the count is 1?

Because that's the old expectCall, and it is unclear how Foundry will handle it in the long term. They are still scrambling to fix all breaking changes introduced recently; see the convo in foundry-rs/foundry#4845.

There's a whole slew of issues caused by using the count parameter explicitly, but, for brevity, I will not dive into all of them here. See my comments at foundry-rs/foundry#4845 (comment) for more details.

I will stick with the count-free version of expectCall for the cases when a single call is expected.

[andreivladbrg]

To make an analogy - you don't change all door locks in your house because your front door needs to have a particular lock for whatever reason.

I'm going to respond with an analogy as well. If you change the doors that the locks are used on, and the locks are no longer compatible, then you'll need to replace the locks than trying to modify the old locks to "work" with the new doors.

you would have to cast the call count to uint64 if you change the function type.

It would be made automatically.

[PaulRBerg]

If you change the doors that the locks are used on, and the locks are no longer compatible, then you'll need to replace the locks than trying to modify the old locks to "work" with the new doors.

You did not continue the analogy in a clear way, as you did not specify what "doors" you are referring to (non-front-door or front-door, or something in between).

Also - why bother with using a non-standard door lock (uint64) when there is a universal lock that everybody uses (uint256)? Why give yourself homework in the long half to retrofit all future uses of these helpers to make them work with uint64, when we know that by using uint256, they would always work as expected?

It would be made automatically.

that's impossible to know beforehand because of Turing-completeness.

[PaulRBerg]

Anyway, for the sake of moving along with this PR, I am happy to concede this point and use uint64 in those expect call helpers - it looks like you have a strong opinion.

However, if we do this, you will have to bear the responsibility for fixing any future issues arising from incompatibilities between uint64 and uint256.

[PaulRBerg]

@andreivladbrg - I pushed a commit to change the type of BATCH_SIZE and count to uint64.

I still think this is a mistake because (again) we don't set the type of generic variables like BATCH_SIZE based on particulars, but I'm happy to live with this mistake since it's a small one in the grand scheme of things.

Good to merge now?

[andreivladbrg]

Good to merge now?

Yes