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

Compute 4 Byte selector from SolError and SolFunction #24

Merged
merged 11 commits into from
Nov 18, 2024

Conversation

willco-1
Copy link
Contributor

This aims to solve #6. I think that there might be a better way to do this, perhaps. Also, I am sure there will be stylistic things to change here.. so I am opening it as a Draft Pr. Please advise on any notes.

@Philogy
Copy link
Contributor

Philogy commented Nov 6, 2024

Just saw this, reviewing.

Comment on lines 63 to 64
func: SolFunction,
err: SolError,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this function take both a func and err? It should be either or

DynSolType::Address => "address",
DynSolType::Uint(_) => "uint256",
DynSolType::String => "string",
_ => panic!("Unsupported type: {:?}", arg.0),
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't support all types. I think Alloy might have a built-in way, try .to_string()

@@ -53,3 +56,34 @@ pub fn u256_as_push(value: U256) -> Opcode {
_ => unreachable!(),
}
}

type Selector = (FixedBytes<4>, FixedBytes<4>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unused

@willco-1 willco-1 marked this pull request as ready for review November 13, 2024 14:50
@Philogy
Copy link
Contributor

Philogy commented Nov 15, 2024

Hey @willco-1, thanks for the contribution. Looking great. Before merging in could you do a few more small things:

  • Remove the redundant closure
  • Write a simple test or two
  • Do pub mod utils in lib.rs to satisfy clippy (run cargo +nightly clippy --workspace --all --all-features --locked -- -D warnings to check for yourself)

thanks!

@Philogy
Copy link
Contributor

Philogy commented Nov 16, 2024

Nice, almost there. Could you get the tests to pass now?

@Philogy
Copy link
Contributor

Philogy commented Nov 17, 2024

Clippy 🙃

@Philogy
Copy link
Contributor

Philogy commented Nov 17, 2024

Also fixed CI so that you should be able to run it yourself, apologize for the small back and forth 😅

@Philogy
Copy link
Contributor

Philogy commented Nov 17, 2024

Closes #6

@Philogy Philogy merged commit 679a436 into huff-language:main Nov 18, 2024
2 of 3 checks passed
@Philogy
Copy link
Contributor

Philogy commented Nov 18, 2024

Merged in, decided to fix the nit myself. Thanks for the 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.

2 participants