The longest post so far, day 9 covers a huge amount of work. This is about 2500 words with 40+ images and is the day I fixed the Foundry errors. Skip to the end if you just want that

Hi everyone, welcome back. Today is day 9 of 30 and I am determined to make this work. Let’s go.

Primary activities for today are:

- Simplifying errors
- transactionId does not exist tests
~~- Tests for simple, complex, custom and other decision cases.

• Shift onlyOwner implementation to OZ's one rather than the Owned.sol I am using now.~~

• Reentrancy attack tests for all basic functions
• Tests for multiple transactions in a row with different outcomes.
• Edge cases for addresses recipient and depositor being the same

This is a lot of work. This is like 3 days of work in 1 day but I feel that I need to catch up with the time lost due to errors and Foundry issues. I am not going to backdate a day or two with interesting content - this project is going to be honest and transparent. I need to do the above items so that I can:

• Test whether adding tokenAddress to a transaction struct adds much gas (and if I can somehow pack it in the struct such that it inits to 0 / does some low cost operation when it is a pure ETH transaction)
• Optimise gas costs (if I am duplicating the code)

The pathway here looks like this:

1. Finish off what I have now
2. See if I can / should extend the stuff I have to ERC20
3. Implement ERC20 however I need to
4. See if I can / should extend the stuff I have to ERC721
5. Implement ERC721 however I need to
6. (Not sure) see if I can / should extend the stuff I have to ERC1155

Let's work on it!

Simplification of errors

Let's look at how Solidity handles custom errors per the docs:

Solidity documentation shows errors which:

• Are not function specific by name (i.e. don’t have the function name in them)
• Do not have 'error' at the end of their names
• Use CamelCase
• Pass back a variable when required
• Have NATSPEC comments above them
• Make sense when read as a statement (they 'sound like' a reply, for example "Bid not high enough") and when 'error' is appended to them it makes sense ("Bid not high enough error").

Let's compare them to my currently horrible error types:

I've bunched these by function name. Time to fix that. This will involve rewriting the main contract as well as the test contract, since I am using ExpectRevert in the test contract with selector values which rely on the error name.

I will follow the style that the Solidity documentation errors use:

Much cleaner! I am not sure if I want to return the actual state of the transaction since the user / frontend could just query the object using getTransactionObject.

Errors are also marginally cheaper when used without a variable, so I will remove it. There is no need to tell the user when they could just query it. Time for the hard part, changing the errors. Luckily I all the old names had error in the name so I can just find them easily and replace them.

I know for a fact that my tests work with my very specific error names so I am confident that they will also work for the less specific names.

Side note: This way of checking if the transaction exists irks me a little as it doesn’t make logical sense given the variable names.

Okay all of the error names in Arbiter.sol have been changed. Time to change them in the test contract.

Changed those too. Let's make sure it all runs:

Nice! That's done.

Checking whether a transaction exists or not

So let's talk about this.

Solidity has no concept of undefined or null. The default value for a created variable is 0. We need to work out whether something exists or not, so let's find out a way to do that.

Using a boolean

Using a boolean would cost extra gas but would be pretty human readable. It wouldn't be the best approach though.

Using a variable which in reality is never zero and checking if it is zero

In this case I would use expiryTime as it is a uint64 which can never be zero in real life (block.timestamp > 0).

We could also use balance but then empty transactions (claimed) would also show as not found. There is no need to have transactionId as part of the object, I will remove that.

Given that space we just got back, I might be able to add something like a uint8 which would store a 1 or a 0. Not sure yet.

Why uint8?

A bool is 8 bits. I don't know why when it could be 1 bit. I'm sure the Solidity devs have a good reason that I’m not aware of.

Let's just pack a isCreated boolean into the struct sin

LlamaPay is a newly released and well optimised Sablier-like service.

They have good code (Etherscan) which I can copy for optimisation. They are also very optimised (although this contract is more complex than a stream function and they have a uint216 max amount rather than uint256 which I want to use).

Streaming services are pre-approved for withdrawal and don't have the DAO element we are planning to build, although there is much by way of optimisation we can learn from LlamaPay.

LlamaPay is token only (whereas this implementation of Arbiter is ETH + token + NFTs) so I may be limited as to how I can copy them.

Back on track...

Let's stay gas optimised and use the expiryTime as a way of checking if something exists. The tests I write will let me know if that fails for whatever reason. I will confirm what I want to do here later. First let's write the main ETH implementation.

More tests

Gas reports aren't working for whatever reason. Foundry issue, I'll fix that later.

Let's write tests for if the transaction doesn't exist. This is for any withdrawal function but not the Other function. The other function may need to be able to withdraw from an address which has a bug where the transaction is determined not to exist.

This test for whether the transaction exists or not will add gas. How much, I don't know. It really doesn't stop much since the person won't be able to match their msg.sender address with the recipient and have the enum transactionState be Claimable. Even if they do, it won't do anything since when the transaction is created it will overwrite this value. I can decide whether I want to keep this later but it is good for comprehensive testing. I can cut out the checks in the contract though and leave the test in to make sure it still reverts (and let it revert with incorrect caller + have it spoof the caller / zero address and let it revert for incorrect state).

Simple and Complex resolution withdrawal tests

As a refresher, simple is the council choosing one side or the other. Complex is the council choosing a side plus a split percentage.

Here is what we have so far. I am getting this error:

An aside on vm.startHoax

Because I keep getting these fallback errors ( the address receiving the Wei is either having an arithmetic overflow or something similar) I am going to make two changes:

1. New variable for balances called SAFE_MAX_VALUE which is roughly half of the uint256 max value (safe max is 2^255 - 1) so if it is doubled it still won't arithmetic overflow.
2. Amount per transaction won't be more than SAFE_MAX_VALUE by our assumption (for testing only) - this works fine since 2^255 - 1 is half of the total number of tokens which can exist and I've seen it work most of the time with this number, just not all of the time.

Wtf is going on now...

I am running the tests (all) maybe 10 times and 1 in 10 will produce either this error:

This is a big mismatch:

or this error:

Let's tackle these.

Error number 1: Mismatched balance

Ignore the horrible code, I'm trying to figure out what's going on here.

I put some logs inside the actual transfer bit of the function:

The fallback function echos the message that it received the number of Wei ending in 4000. This sounds right to me. Note this contract keeps up to 999 Wei per transaction. This is because we have to in order to do calculations. I can add in a check that the fee isn’t zero before I send if that helps with predictability.

But then it logs the expected balance as ending in 4000 but it actually received more than that.

Let's figure out the relationship between these two numbers.

It's virtually zero compared to SAFE_MAX_VALUE. So that eliminates the possibility that it's a rounding error or something.

Checking the fallbacks, it seems like the contract is sending out the correct amount. Why is this?

But the testing contract sees a different number.

It also can't be going to the depositor (I checked)...

FoundryUp time

So since I don't know wtf is going on here, I am going to run foundryup

Nothing changed.

I am now going to compare the contract balances from within the test function. The fact that hundreds of fuzz tests succeed but sometimes it fails tells me that it's probably a Foundry bug. It succeeds even when the fuzz count is 512 or more.

More investigation

This above screenshot will mean nothing to you. There are certain bits which match up (the numbers ending in 0000) but somehow the recipient balance has increased by some huge number even though the contract has not lost any extra money. Since I don't have a clue how this is happening, I have one more guess.

I think that the deposit address being reused was a recipient address from before and it is keeping balances from old fuzz runs.

Let's investigate:

Nope, looks like it starts with zero balance...

Let's throw a bunch of those in the code to see if the balance ever becomes non-zero before the transfer event occurs:

Finally finding it

Okay so it looks like it is starting off with non-zero balance. May be reusing stuff from other fuzz runs (address collision?) or something else. Hi, me from day 10 here. It was just having a balance from previous fuzz runs or some other reason. I don’t think it was necessarily starting with a non-zero balance.

Anyway a vm.assume(someAddress.balance == 0) should fix this. Let's try:

This wasn't enough, it kept happening. Then I realised why.

Foundry was reusing the addresses from previous tests which included the test contract address but the test contract has a balance by default. This was causing arithmetic overflow within the fallback function of the test contract after a number of fuzz runs deposited into the same address.

I have now added this:

Awesome! 10,000 fuzz runs on everything passes now:

I also posted this to Twitter:

I am a bit frustrated that it took me this long to find and fix this. It is fixed now.

Back on track

Okay, now that I know that, let's go full steam. I need another coffee.

This is the only error I get now:

Let's add a helper function which adds those rules for times when we are comparing balances:

When we run this it should allow us to compare balances before and after and not have to worry about stuff. Let’s see if it runs:

Test for the Simple decision withdrawal

I have written this too (it is a simplified version of complex decision withdrawal):

I am mostly happy with that level of testing for these so far.

Get Transaction Object tests

While I am in here I made it so that the transaction object request call reverts if expiryTime is 0 (the transaction does not exist).

I also wrote a test for this:

OpenZeppelin’s Ownable Contract

I am currently using Owned.sol which I found online but it isn't going to be as good as OZ's code.

Let's move over.

This response is just the expectRevert() statements getting a new input and freaking out. Let's calm them down.

Changing vm.expectRevert(); to vm.expectRevert("Ownable: caller is not the owner"); does the trick! We just needed to match the string to what OZ's contract returns and everything is running again.

Tests for revert - simple and complex outcomes

OKAY, I have finally added tests for simple and complex withdrawal failure conditions (incorrect caller, incorrect state, transaction ID is incorrect). There is overlap with the zero balance error (i.e. the incorrect state error will always cover it) so I am thinking of removing that with the exception of for depositEth.

Conclusion

Wow, what a day! Lots of test writing and not much by way of the main contract changing - but that's okay! When I introduce tokens to this environment I will be glad I did this since I can just copy and paste the tests.

The only time I will have to write more tests is for ERC721 when we get there. I'm really happy with my progress today - I finally feel like I've overcome the Foundry learning curve and am on the other side, where it's nice and peaceful and not full of 100 errors.

Today was a great day! See you tomorrow!