Skip to content

feat(params): adding error case in UnmarshalInto func#70

Open
Caroline-theotter wants to merge 6 commits intomasterfrom
feat/adding_functions_params.go
Open

feat(params): adding error case in UnmarshalInto func#70
Caroline-theotter wants to merge 6 commits intomasterfrom
feat/adding_functions_params.go

Conversation

@Caroline-theotter
Copy link
Copy Markdown
Contributor

@Caroline-theotter Caroline-theotter commented Aug 11, 2022

Current situation

curl https://polygon-mainnet.infura.io/v3/e35433ff37cb4c80aac3bc1afc8c0d08 --data '{
"id": 1,
"jsonrpc": "2.0",
"method": "eth_getLogs",
"params": [{
"fromBlock":"26843920",
"toBlock": "26843921",
"address": "0xc2132d05d31c914a87c6611c10748aeb04b58e8f",
"topics":["0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef"]
}]
}'

{
"jsonrpc": "2.0",
"id": 0,
"error": {
"code": -32602,
"message": "quantity values must start with 0x"
}
}

Expected situation

curl https://polygon-mainnet.infura.io/v3/e35433ff37cb4c80aac3bc1afc8c0d08 --data '{
"id": 1,
"jsonrpc": "2.0",
"method": "eth_getLogs",
"params": [{
"fromBlock":"26843920",
"toBlock": "26843921",
"address": "0xc2132d05d31c914a87c6611c10748aeb04b58e8f",
"topics":["0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef"]
}]
}'

{
"jsonrpc": "2.0",
"id": 0,
"error": {
"code": -32602,
"message": "invalid argument 0: quantity values must start with 0x"
}
}

Adding the indexation of the invalid argument

@Caroline-theotter Caroline-theotter force-pushed the feat/adding_functions_params.go branch from 733dcc7 to 9d104f6 Compare September 15, 2022 17:43
@Caroline-theotter Caroline-theotter marked this pull request as ready for review September 27, 2022 14:14
@basgys
Copy link
Copy Markdown
Contributor

basgys commented Sep 27, 2022

@Caroline-theotter I went through the PR, and I see a few red flags (reflect and the low-level JSON manipulation). It does not necessarily mean it is bad, but I need more context to properly review this PR.

Could you show me a simple example from your test case to fully understand the behavioural difference before and after your feature?

@@ -92,6 +98,22 @@ func (p Params) UnmarshalInto(receivers ...interface{}) error {
return errors.New("not enough params to decode")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this check required anymore? if you pass more receivers than params, isn't it enough to set them to nil? That seems to be the intended behavior copied across from go-ethereum library at line 169

Copy link
Copy Markdown
Contributor Author

@Caroline-theotter Caroline-theotter Oct 3, 2022

Choose a reason for hiding this comment

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

not sure to understand @ggarri

@Caroline-theotter Caroline-theotter force-pushed the feat/adding_functions_params.go branch from 85345e6 to 7dcebde Compare October 4, 2022 13:41
@basgys
Copy link
Copy Markdown
Contributor

basgys commented Oct 5, 2022

The only difference in the expected behaviour lies in the prefix invalid argument 0: .

I find this change quite heavy for an outcome that is unclear. This code is used in several projects and sometimes in hot paths, so I think an explicit motivation for this change and perhaps a benchmark to show how it performs would be great.

If you intend to mimic the behaviour of a specific implementation (e.g. Geth), then I would question if this is the right place for it. go-ethlibs is a generic implementation for json-rpc that can be used with any EVM-compatible chains.

@Caroline-theotter
Copy link
Copy Markdown
Contributor Author

Caroline-theotter commented Oct 11, 2022

@basgys I understand you POV.
But can you give an example where that change can be problematic ?

And tell me if I'm wrong but that change can be a problem if a hardcoded error is in a condition, right ?

I checked in the infura repo where I can find, for example, the hardcoded error "quantity values must start with 0x" and found it only in return response not in condition.

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