Skip to content

Add array and tuple params support #1

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nishuzumi
Copy link
Contributor

Added temporary solution for arrays and tuples in parameters

@nand2
Copy link
Collaborator

nand2 commented Aug 2, 2024

Hi!

Thanks for your work on this, I have written my thoughts of the specification itself at ethereum magicians here :
https://ethereum-magicians.org/t/eip-4804-web3-url-to-evm-call-message-translation/8300/76

Regarding the code itself, until we all agree on the specification, I will just comment on 2 things :

  • For the tests, please use the same system that is currently used. (If the tests you have written are temporary, I can understand they can be useful for quick testing). Our test suite, common to both the Go implementation and the Javascript implementation, is at https://github.com/web3-protocol/web3protocol-tests/ and is used in this go module via a git submodule (see the Testing section of the readme)

  • Please do not use a code formatter on existing code that you do not edit : It make the diff difficult to read, see : https://github.com/web3-protocol/web3protocol-go/pull/1/files

I will not comment yet on the code itself until we all agree on the specification discussed at ethereum magicians :)

Thanks for working on this, this is a non trivial item!

@nishuzumi
Copy link
Contributor Author

Before proceeding, I recommend formatting the code using tools like gofmt. Currently, the code lacks a standard format, making it difficult to maintain a consistent coding style.

@qizhou
Copy link

qizhou commented Aug 3, 2024

Before proceeding, I recommend formatting the code using tools like gofmt. Currently, the code lacks a standard format, making it difficult to maintain a consistent coding style.

I would recommend creating another issue/PR to track/address formatting. Putting the new feature and reformat in a single PR is difficult to review.

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.

3 participants