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

feat: support ethabi Contract methods #195

Merged
merged 19 commits into from
Jul 29, 2023

Conversation

andyrobert3
Copy link
Contributor

@andyrobert3 andyrobert3 commented Jul 18, 2023

Motivation

Fixes #101

Struggling to write tests for load method from ethabi
Thanks @prestwich for the help!

Solution

Reference from methods from ethabi

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@andyrobert3 andyrobert3 changed the title feat: support ethabi Contract methods feat: support ethabi Contract methods Jul 26, 2023
@andyrobert3
Copy link
Contributor Author

andyrobert3 commented Jul 26, 2023

@prestwich @DaniPopes

When writing tests for .load() i face this issue

value: Error("invalid type: string "address", expected a valid internal type", line: 1, column: 250)'

I used the same implementation as ethabi
https://docs.rs/ethabi/latest/src/ethabi/contract.rs.html#167-169

let file_path: String = format!("tests/{}", path);
let file: File = File::open(file_path).unwrap();
let buffer: BufReader<File> = BufReader::new(file);
let loaded_abi: JsonAbi = JsonAbi::load(buffer).unwrap();

Do you know how to solve this?
Why does include_str! work with string parsing but not run time File::open with BufReader

Tried adding Basic(String) into InternalType, to no avail

pub enum InternalType {
    // ... 
    Basic(String),
}

@prestwich
Copy link
Member

prestwich commented Jul 26, 2023

value: Error("invalid type: string "address", expected a valid internal type", line: 1, column: 250)'

this seems odd. can you link to the ABI file that you're using for tests?

nevermind i am dumb it's in the folder already

@prestwich
Copy link
Member

prestwich commented Jul 26, 2023

okay i have tracked this down.

  • InternalType is deserialized by borrowing data from the deserializer with Visitor::visit_borrowed_str
  • serde_json::from_reader configures the Deserializer to not call Visitor::visit_borrowed_str (why?)
  • Instead it calls Visitor::visit_string which implicitly calls Visitor::visit_str.
  • Neither of those are implemented for the ItVisitor.
  • Because the deserializer calls an unimplement vist_ method, it returns the unexpected error, even tho the input is actually expected

@prestwich
Copy link
Member

prestwich commented Jul 26, 2023

@andyrobert3 if you don't mind, I can push a fix to your branch. the fix will be loading the entire file into a string and then chaning to read_string, as well as making serde_json::from_reader always error with a clear message

@andyrobert3
Copy link
Contributor Author

sure @prestwich feel free to push!

need to sleep since its a bit late here

@prestwich
Copy link
Member

cool i pushed. this solves the immediate issue, and should make it clearer for future users :)

@andyrobert3 andyrobert3 marked this pull request as ready for review July 27, 2023 15:44
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Cargo.toml Outdated Show resolved Hide resolved
crates/json-abi/src/abi.rs Outdated Show resolved Hide resolved
crates/json-abi/src/abi.rs Outdated Show resolved Hide resolved
@prestwich
Copy link
Member

needs cargo fmt

@prestwich
Copy link
Member

@DaniPopes mind re-reviewing with my additions? :)

@prestwich prestwich requested a review from DaniPopes July 29, 2023 16:12
@DaniPopes DaniPopes merged commit ca9ec68 into alloy-rs:main Jul 29, 2023
18 checks passed
@andyrobert3
Copy link
Contributor Author

Was going to work on the requested changes today, but thanks to @prestwich @DaniPopes for reviewing and making the required changes

@prestwich
Copy link
Member

yeah, hope you don't mind us getting it over the finish line. thank you for your contribution! 🎉

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

Successfully merging this pull request may close these issues.

Add more methods to AbiJson and items
3 participants