refactor: use custom errors to optimize gas costs

[0xb337r007]

This PR updates the Marketplace and Proofs contract to use custom errors and revert instead require + "string" to reduce the gas costs on reverts.

[0x-r4bbit]

Changes LGTM

[markspanbroek]

Looks great, thanks! Today I learned about custom errors in Solidity :)

[markspanbroek]

@AuHau and @emizzle: I'd like to merge this, but it changes the errors that are returned (in a good way). The downside is that it might require adding support for Solidity errors in nim-ethers... Is this something that we can handle right now, given the other work that we have?

[0x-r4bbit]

it might require adding support for Solidity errors in nim-ethers

@markspanbroek can you elaborate on this? Is there anything in particular nim-ethers does with errors that could break with this?

Custom errors in solidity are just the first 4 bytes of their keccak256(ErrorType).

For nim-ethers this means, instead of emitting the bytes of the error message, it emits the bytes of the error type. I'd assume this to just-work™ ?

[markspanbroek]

Is there anything in particular nim-ethers does with errors that could break with this?

Currently it forwards error strings that it gets from a json-rpc call to an ethereum node. I haven't checked whether this just-works:tm: with hardhat and geth, but I'd be surprised if it did. Someone needs to do a conversion from the keccak256(ErrorType) bytes to a string, and I don't think hardhat and geth can do that because they don't have access to the contract ABI.

[emizzle]

ethers will definitely require some work to abi decode the error. Additionally, I believe the functionality that obtains the revert reason will need to be updated as well.

Would it be ok if we hold off on merging this until we can free up some time to update ethers first?

[0x-r4bbit]

ethers will definitely require some work to abi decode the error.

Okay, so I guess this depends on what the expectation is. The data nim-ethers receives is 4byte(keccak(errortype)). If you want nim-ethers to propagate this as, say "MyCustomError" then it will have to a) know all the custom error types of the contract and b) do the 4byte(keccak(errortype)) on them so it knows which custom error it is dealing with, just so that it can emit it as "MyCustomError".

I assume nim-ethers is more like a library that other projects use for their tools and apps.
Do you as someone who builds such a tool expect nim-ethers to emit custom error types as strings?

I'd rather expect it to just emit the 4byte(keccak(errorType)) and let my tool/app decide what to do with it.
E.g. front-end apps would need to maintain a map of 4byte(keccak(errorType)) => some user error message to handle custom errors.

Anyways, I'm not arguing against it, just want to make sure we understand that nim-ethers would do work that should probably be offloaded to apps.

[markspanbroek]

Good question @0x-r4bbit. For me, the logical thing that nim-ethers should do with custom errors is allowing them to be declared, just like it allows events to be declared. Then it should have enough information to convert the 4 bytes into an error message.

But even if we don't do this, and return the four bytes, then it still needs some work, because right now we're returning a string, not bytes.

Would it be ok if we hold off on merging this until we can free up some time to update ethers first?

Yes, let's do the update to nim-ethers first, then merge this.

[0x-r4bbit]

Hey friends, what's the state of this PR here?

[AuHau]

Hey friends, what's the state of this PR here?

We have implementation for the custom errors in Nim thanks to @markspanbroek (codex-storage/nim-ethers#69). We just have to merge it all the way to nim-codex and then we can integrate it!

[0x-r4bbit]

Wonderful looking forward to it