feat(node): parse contract revert

[cryptoAtwill]

This PR closes #1155.

In ipc-actor-abi build process, it generates the code to scan the target contracts' abi errors and generate a static mapping that mapping error selector to the ethers AbiError. Then in the JsonClient, extracts the error message from contract and parse it against the error mapping. Reason for favouring JsonClient over middleware is inner middleware modifies the original error message and does not expose the contract revert data directly which makes obtaining the data string difficult.

On my local computer, I didn't notice any difference in build speed from the extra step.

One downside of the current approach is that the middleware modifies or expects the error data in a specific format, so the custom JsonClient does not seem able to modify the error bytes (maybe there is a way to bypass middleware expections?) and instead can just log an error.

---

This change is 

[drahnr]

Reviewable status: 0 of 8 files reviewed, 7 unresolved discussions

---

contract-bindings/build.rs line 194 at r1 (raw file):

}

fn error_mapping_gen(mod_f: &mut fs_err::File, all_contracts: &[&str]) -> color_eyre::Result<()> {

This definitely needs documentation. It should also take an impl std::io::Write, and make a unit test to supplement documentation

---

contract-bindings/build.rs line 200 at r1 (raw file):

        .iter()
        .map(|s| {
            // Need to convert contract name to

Is this a TODO or an explanation?

---

contract-bindings/src/error_macro.rs line 8 at r1 (raw file):

            use ethers::utils::hex;

            lazy_static::lazy_static! {

If we anticipate this being used outside this crate, then we should ensure the lazy_static is also re-exported and use crate-name crate as a prefix operation.

---

contract-bindings/src/error_macro.rs line 9 at r1 (raw file):

            lazy_static::lazy_static! {
                static ref MAP: std::collections::BTreeMap<String, ethers::abi::ethabi::AbiError> = {

For std use ::std in macro_rules

---

contract-bindings/src/error_macro.rs line 32 at r1 (raw file):

            impl ContractErrorParser {
                pub fn parse_from_bytes(bytes: &[u8]) -> anyhow::Result<Option<String>> {

I'd expect an error type with a specific variant for UnknownErrorType{ source: String } variant.

---

contract-bindings/src/error_macro.rs line 51 at r1 (raw file):

                pub fn parse_from_hex_str(err: &str) -> anyhow::Result<Option<String>> {
                    let bytes = hex::decode(err)?;

const_hex is a possilby preferable impl

---

contract-bindings/src/error_macro.rs line 94 at r1 (raw file):

            None
        );
    }

I really really like the automatism!

[drahnr]

Reviewable status: 0 of 8 files reviewed, 8 unresolved discussions (waiting on @cryptoAtwill)

---

contract-bindings/src/error_macro.rs line 21 at r2 (raw file):

                    for (_, v) in errors.iter() {
                        for i in v {
                            let selector = ethers::utils::hex::encode(&i.signature().0[0..SOLIDITY_SELECTOR_BYTE_SIZE]);

Needs additional documentation why we use the prefix only of the hex encdoded and some spec ref.

[drahnr]

Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @cryptoAtwill)

---

contract-bindings/build.rs line 202 at r2 (raw file):

            let snake_case = camel_to_snake(s);
            let upper_case = s.to_uppercase();
            format!("[{snake_case}, {upper_case}_ABI]")

Smells like this could be a function call just as well?

[cryptoAtwill]

Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @drahnr)

---

contract-bindings/build.rs line 194 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

This definitely needs documentation. It should also take an impl std::io::Write, and make a unit test to supplement documentation

added doc

---

contract-bindings/build.rs line 200 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

Is this a TODO or an explanation?

explanation, but I removed it, should be clear from the naming.

---

contract-bindings/build.rs line 202 at r2 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

Smells like this could be a function call just as well?

updated

---

contract-bindings/src/error_macro.rs line 8 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

If we anticipate this being used outside this crate, then we should ensure the lazy_static is also re-exported and use crate-name crate as a prefix operation.

I think MAP should just be internal

---

contract-bindings/src/error_macro.rs line 32 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

I'd expect an error type with a specific variant for UnknownErrorType{ source: String } variant.

I think if the error cannot be parsed, we should just return as it is instead of throwing an error. Because contract can call other contracts and probably we dont have the abi for the called contract. It's not an error, just some information we don't have. This will be so for cross message calls.

---

contract-bindings/src/error_macro.rs line 51 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

const_hex is a possilby preferable impl

updated

---

contract-bindings/src/error_macro.rs line 21 at r2 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

Needs additional documentation why we use the prefix only of the hex encdoded and some spec ref.

added

[drahnr]

Reviewable status: 0 of 9 files reviewed, 8 unresolved discussions (waiting on @cryptoAtwill)

---

contract-bindings/src/error_macro.rs line 8 at r1 (raw file):

Previously, cryptoAtwill wrote…

I think MAP should just be internal

I am not talking about the usage of the static variableMAP but about the symbol resolution of i.e. lazy_static at call site.

---

contract-bindings/src/error_macro.rs line 32 at r1 (raw file):

Previously, cryptoAtwill wrote…

I think if the error cannot be parsed, we should just return as it is instead of throwing an error. Because contract can call other contracts and probably we dont have the abi for the called contract. It's not an error, just some information we don't have. This will be so for cross message calls.

That's a question of the call-site not of the return type. The return type should allow usage of either, a Result<_> type allows that with .ok().

---

contract-bindings/src/error_macro.rs line 14 at r3 (raw file):

                    $(
                        errors.extend($crate::gen::$snake_case::$snake_case::$abi.errors.clone());

If this the only reason for using a macro, we could just as well use a function and generate the function call invocation, which would simplify this greatly and remove lot's of the above concerns

---

contract-bindings/build.rs line 196 at r3 (raw file):

/// generate the mapping between contract error selectors to the ethers abi error type for parsing
/// potential contract errors.
/// This function generates a rust file that creates the error mapping, see `gen_contract_error_mapping`

nit: Please use links for symbols and capitalize terms like ABI and word starts. This will become public rendered documentation.

[drahnr]

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @cryptoAtwill)

---

contract-bindings/src/error_macro.rs line 9 at r1 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

For std use ::std in macro_rules

LGTM

---

contract-bindings/src/error_macro.rs line 14 at r3 (raw file):

Previously, drahnr (Bernhard Schuster) wrote…

If this the only reason for using a macro, we could just as well use a function and generate the function call invocation, which would simplify this greatly and remove lot's of the above concerns

LGTM

---

contract-bindings/src/error_parser.rs line 67 at r5 (raw file):

    pub fn parse_from_hex_str(err: &str) -> Result<String, ParseContractError> {
        let bytes = const_hex::hex::decode(err).map_err(|e| ParseContractError::ErrorNotHexStr(e.to_string()))?;

nit: We don't need to stringify here, using a #[source] const_hex::FromHexError

[drahnr]

Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on @cryptoAtwill)

[karlem]

I guess we are confident now that the data is there?

[cryptoAtwill]

yep, should be. If not nothing we can do.

[karlem]

Just couple of nits and comments. But otherwise it looks good!

[karlem]

@cryptoAtwill The check fails though.