Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Sol* traits' from_rust method to new and remove to_rust #90

Closed
DaniPopes opened this issue Jun 9, 2023 · 14 comments · Fixed by #133
Closed

Rename Sol* traits' from_rust method to new and remove to_rust #90

DaniPopes opened this issue Jun 9, 2023 · 14 comments · Fixed by #133
Labels
enhancement New feature or request

Comments

@DaniPopes
Copy link
Member

No description provided.

@DaniPopes DaniPopes added enhancement New feature or request good first issue Good for newcomers labels Jun 9, 2023
@prestwich
Copy link
Member

to rust is currently used to implement encoding. However, this currently requires cloning all the data -_-

The fundamental issue is that encoding a tuple takes Borrow<(T, U, V, ...)>. This means we can't take (&T, &U, &V, ...). Do we have any idea how to specify encode such that we could accept any of

  • a tuple
  • a reference to a tuple
  • a tuple of references
  • a reference to a tuple of references?
    /// ABI encode the call to the given buffer **without** its selector.
    #[inline]
    fn encode_raw(&self, out: &mut Vec<u8>) {
        out.reserve(self.data_size());
        out.extend(<Self::Tuple as SolType>::encode(self.to_rust()));
    }
    ```

@DaniPopes
Copy link
Member Author

If we can implement the trait for &T generically, then all of the types you listed are valid, and any combinations of them

trait Something {
    fn f(&self);
}

impl<T: Something> Something for &T {
    fn f(&self) {
        T::f(*self)
    }
}

impl Something for u8 {
    fn f(&self) {}
}

impl Something for () {
    fn f(&self) {}
}

impl<T: Something> Something for (T,) {
    fn f(&self) {}
}

impl<T: Something, U: Something> Something for (T, U) {
    fn f(&self) {}
}

fn main() {
    Something::f(&(0u8, ()));
    Something::f(&(0u8, &()));
    Something::f(&(&0u8, &()));
    Something::f(&&(0u8, ()));
    Something::f(&&(0u8, &()));
    Something::f(&&(&0u8, &()));
    Something::f(&&(&&&&&&&&&&&&0u8, &&&&()));
}

@prestwich
Copy link
Member

it's not the trait that's the problem. it's the defintion of encode

 /// Encode an ABI sequence.
    fn encode<B>(rust: B) -> Vec<u8>
    where
        Self::TokenType: TokenSeq,
        B: Borrow<Self::RustType>,

@prestwich
Copy link
Member

we need to find some way to write the bound on B such that it allows any of those

@DaniPopes DaniPopes removed the good first issue Good for newcomers label Jun 11, 2023
@DaniPopes
Copy link
Member Author

DaniPopes commented Jun 11, 2023

We might be able to avoid cloning by adding fn as_rust(&self) -> &Self::Tuple, implemented with a pointer cast in sol!. I don't know if struct { T... } has the same layout as tuple (T,...), so this may not be sound

@prestwich
Copy link
Member

pretty sure that's not sound, or is sound only as an implementation detail. default layout has no guarantees

@DaniPopes
Copy link
Member Author

Yes, the default layout has no guarantees other than the basic soundness stuff, but tuples also use the default layout except for the unit type, but that doesn't matter because empty structs are not allowed here

@DaniPopes
Copy link
Member Author

@prestwich
Copy link
Member

yeah, but we have no guarantee that it remains sound in any given future version, or on any architectures we're not checking. Hence why i say "sound as an implementation detail"

@DaniPopes
Copy link
Member Author

Hmm yeah you're not allowed to do any casting with the default Rust repr, even if the defs are identical. So we can't do that with the default tuples, but we could if we defined a second struct for the fields / generic Tuple struct? Idk

@prestwich
Copy link
Member

we could actually have all sol structs be

pub struct MyStruct(InnerTuple) and thne have named getters? removing property access seems bad tho :(

@prestwich
Copy link
Member

this approach?

#83 (comment)

@prestwich
Copy link
Member

removing to_rust requires #36, due to its use in encode_packed_to

@prestwich
Copy link
Member

for other users to_rust like encode_size we can tokenize and then calculate based on the token, but because packed encoding is inherently type-aware (😠😠😠) the tokens cannot sub in for the actual type

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants