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

Implement msg.sig #311

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Implement msg.sig #311

merged 1 commit into from
Mar 17, 2021

Conversation

georgeroman
Copy link
Contributor

Fixes #278

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

Copy link
Collaborator

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good. Just a tiny nitpick about the casing. Also do you mind creating a file 311.feature.md in https://github.com/ethereum/fe/tree/master/newsfragments with the content being something like:

Support for msg.sig to read the function identifier

This is to ensure the feature gets included in the next release notes.

@@ -0,0 +1,5 @@
contract Foo:
pub def bar() -> bytes[32]:
msgSig: bytes[32]
Copy link
Collaborator

Choose a reason for hiding this comment

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

The idiomatic way to write Fe code is with snake_case. Please change msgSig to msg_sig.

@@ -316,6 +317,19 @@ pub fn bytes_token(s: &str) -> ethabi::Token {
ethabi::Token::FixedBytes(ethabi::FixedBytes::from(s))
}

#[allow(dead_code)]
pub fn bytes32(s: &str) -> Vec<u8> {
H256::from_str(s)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just pointing out that we try to not use single letter variable names such as s. That said, this file violates that rule numerous times and we don't need to fix that in this PR :)

@georgeroman
Copy link
Contributor Author

Addressed all comments.

@cburgdorf cburgdorf merged commit d4833c3 into ethereum:master Mar 17, 2021
@cburgdorf
Copy link
Collaborator

Awesome! Landed 🎉

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.

Implement msg.sig
2 participants