Skip to content
This repository has been archived by the owner on Oct 19, 2024. It is now read-only.

feat(ethers-core/abi): impl AbiArrayType for u8 #1140

Closed

Conversation

meetmangukiya
Copy link
Contributor

@meetmangukiya meetmangukiya commented Apr 13, 2022

Motivation

Had a compilation error earlier in abigen:

1198 |         pub pairs_to_swap: ::std::vec::Vec<(u8, u8)>,
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `AbiArrayType` is not implemented for `u8`

So implementing it.

Solution

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@meetmangukiya
Copy link
Contributor Author

I am not entirely sure if this is breaking anything or any compat. I defer to experts.

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is not the right solution here, because now it won't encode fixed length arrays..need to think about this a bit more.

@mattsse
Copy link
Collaborator

mattsse commented Apr 13, 2022

AbiArrayType is such an ugly hack ...

but the right way to fix this is to implement this manually for tuples of u8

impl AbiArrayType for Vec<(u8, u8)>

impl AbiArrayType for Vec<(u8, u8, u8)>

etc...

@meetmangukiya
Copy link
Contributor Author

@mattsse wouldn't it still fail when there's some tuple like Vec<(u8, T, ...)>?

@mattsse
Copy link
Collaborator

mattsse commented Apr 13, 2022

yes this is a limitation and super ugly.

we could generate all those variants, I believe there is already a macro for tuples

@meetmangukiya
Copy link
Contributor Author

Shouldn't

impl<$($ty, )+> AbiArrayType for ($($ty,)+) where
$(
$ty: AbiType,
)+ {}
implement AbiArrayType for different variant of tuples and on top of that
impl<T: AbiArrayType> AbiArrayType for Vec<T> {}
implement AbiArrayType for Vec<T>?

which should mean we shouldn't need any of these?

impl AbiArrayType for Vec<(u8, u8)> {}

impl AbiArrayType for Vec<(u8, u8, u8)> {}

@meetmangukiya
Copy link
Contributor Author

Pasting complete error for more context:

error[E0277]: the trait bound `u8: AbiArrayType` is not satisfied
    --> mean-finance/src/contracts/dcahub_companion.rs:1198:9
     |
1198 |         pub pairs_to_swap: ::std::vec::Vec<(u8, u8)>,
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `AbiArrayType` is not implemented for `u8`
     |
     = help: the following implementations were found:
               <i128 as AbiArrayType>
               <i16 as AbiArrayType>
               <i32 as AbiArrayType>
               <i64 as AbiArrayType>
             and 5 others
     = note: required because of the requirements on the impl of `AbiArrayType` for `(u8, u8)`
     = note: required because of the requirements on the impl of `AbiType` for `Vec<(u8, u8)>`

I couldn't find this exact trait bound(u8: AbiArrayType) causing this error.

impl<T: AbiArrayType> AbiType for Vec<T> {
fn param_type() -> ParamType {
ParamType::Array(Box::new(T::param_type()))
}
}

would require for T i.e. (u8, u8) to be trait bounded by AbiArrayType.

which it should be because of this implementation

impl<$($ty, )+> AbiArrayType for ($($ty,)+) where
$(
$ty: AbiType,
)+ {}

now, this implementation in turn bounds the tuple element type T to be trait bound by AbiType. However, the error seems to be that T needs to be bound by AbiArrayType. Where is that trait bound coming from?

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this trait is required because of the special Vec<u8> => Bytes condition so it's sole purpose is to not be implemented by u8

need some manual impls for u8 tuples unfortunately

}
}
impl<const N: usize> AbiArrayType for [u8; N] {}
impl AbiArrayType for u8 {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this defeats the whole purpose of this trait.

Copy link
Contributor Author

@meetmangukiya meetmangukiya Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The currently pushed code is certainly wrong. Can you check the comment #1140 (comment) regarding the manual impls for u8 tuples? Also, I don't think we need to implement AbiArrayType for tuples for fixing this error, tuples already have AbiArrayType implemented IIUC

impl<$($ty, )+> AbiArrayType for ($($ty,)+) where
$(
$ty: AbiType,
)+ {}

For the broader issue and source of error #1140 (comment)

@mattsse
Copy link
Collaborator

mattsse commented Apr 13, 2022

Shouldn't

impl<$($ty, )+> AbiArrayType for ($($ty,)+) where
$(
$ty: AbiType,
)+ {}

implement AbiArrayType for different variant of tuples and on top of that

impl<T: AbiArrayType> AbiArrayType for Vec<T> {}

implement AbiArrayType for Vec<T>?
which should mean we shouldn't need any of these?

impl AbiArrayType for Vec<(u8, u8)> {}

impl AbiArrayType for Vec<(u8, u8, u8)> {}

interesting, need to check this manually what's going on here

@mattsse
Copy link
Collaborator

mattsse commented Apr 13, 2022

@meetmangukiya you're of course right: #1143

can you share the whole struct and its derive macros that's causing the issue, perhaps we're generating wrong trait bounds somewhere

@meetmangukiya
Copy link
Contributor Author

    #[derive(
        Clone,
        Debug,
        Default,
        Eq,
        PartialEq,
        ethers :: contract :: EthCall,
        ethers :: contract :: EthDisplay,
    )]
    #[ethcall(
        name = "getNextSwapInfo",
        abi = "getNextSwapInfo(address[],(uint8,uint8)[])"
    )]
    pub struct GetNextSwapInfoCall {
        pub tokens: ::std::vec::Vec<ethers::core::types::Address>,
        pub pairs: ::std::vec::Vec<(u8, u8)>,
    }
    #[derive(
        Clone,
        Debug,
        Default,
        Eq,
        PartialEq,
        ethers :: contract :: EthCall,
        ethers :: contract :: EthDisplay,
    )]
    #[ethcall(
        name = "swap",
        abi = "swap(address[],(uint8,uint8)[],address,address,uint256[],bytes)"
    )]
    pub struct SwapCall {
        pub tokens: ::std::vec::Vec<ethers::core::types::Address>,
        pub pairs_to_swap: ::std::vec::Vec<(u8, u8)>,
        pub reward_recipient: ethers::core::types::Address,
        pub callback_handler: ethers::core::types::Address,
        pub borrow: ::std::vec::Vec<ethers::core::types::U256>,
        pub data: ethers::core::types::Bytes,
    }

@meetmangukiya
Copy link
Contributor Author

Just rebased my abigen binary over latest ethers-rs and the generated code compiles fine 💀. I apologise for the wasted time @mattsse.

New code looks like

    #[derive(
        Clone,
        Debug,
        Default,
        Eq,
        PartialEq,
        ethers :: contract :: EthCall,
        ethers :: contract :: EthDisplay,
    )]
    #[ethcall(
        name = "getNextSwapInfo",
        abi = "getNextSwapInfo(address[],(uint8,uint8)[])"
    )]
    pub struct GetNextSwapInfoCall {
        pub tokens: ::std::vec::Vec<ethers::core::types::Address>,
        pub pairs: ::std::vec::Vec<PairIndexes>,
    }

@mattsse
Copy link
Collaborator

mattsse commented Apr 13, 2022

great!
just added this to confirm: #1144

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants