Skip to content

Add array and tuple params support#1

Open
nishuzumi wants to merge 1 commit into
web3-protocol:mainfrom
nishuzumi:main
Open

Add array and tuple params support#1
nishuzumi wants to merge 1 commit into
web3-protocol:mainfrom
nishuzumi:main

Conversation

@nishuzumi
Copy link
Copy Markdown
Contributor

Added temporary solution for arrays and tuples in parameters

@nand2
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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