This repository has been archived by the owner on Nov 6, 2020. It is now read-only.
[light/jsonrpc] Provide the actual account for eth_coinbase RPC and unify error handeling for light and full client
#9383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
niklasad1
added
A0-pleasereview 🤓
|
@niklasad1 Do you think it would be a good idea to align full client and light client behavior? Change light client to return zero address when not found, or change full client to return error when not found? |
|
Yeah, I think we should align the behavior in the full and light client but I don't like the behavior of returning the zero address to indicate errors. So, I would want to return errors in both clients however it might make the |
sorpaas
added
the
B7-releasenotes 📜
|
Yeah, we should have both methods aligned. I'm ok to include that in the relnotes, "the RPC spec" doesn't really describe error conditions at all (I guess that's the reason we didn't return many errors in the past), so there is much room for improvement here. I'm ok with returning an error, but we should also document all the error conditions in wiki in some nice readable and consistent format. |
dvdplm
reviewed
Aug 28, 2018
| @@ -252,7 +252,15 @@ impl<T: LightChainClient + 'static> Eth for EthClient<T> { | |||
| } | |||
|
|
|||
| fn author(&self) -> Result<RpcH160> { | |||
There was a problem hiding this comment.
Just for my education: what is the reason for rpc-specific hashtypes like RpcH160?
There was a problem hiding this comment.
Historical reasons - hash types were not JSON-serializable in the past, I think currently we could get rid of the wrappers and just use the regular types
niklasad1
force-pushed
the
light/provide-account-eth_coinbase
branch
from
45dc28b
to
cda2a76
Compare
niklasad1
changed the title
eth_coinbase RPCeth_coinbase RPC and unify error handeling for light and full client
niklasad1
force-pushed
the
light/provide-account-eth_coinbase
branch
from
cda2a76
to
0150a2f
Compare
The previous implementation always provided the `zero address` on `eth_coinbase` RPC. Now, instead the actual address is returned on success or an error when no account(s) is found!
In the full-client return an error when no account is found instead of returning the `zero address`
niklasad1
force-pushed
the
light/provide-account-eth_coinbase
branch
from
0150a2f
to
bc05784
Compare
…ount-eth_coinbase
sorpaas
approved these changes
Sep 5, 2018
ordian
added
A8-looksgood 🦄
cheme
approved these changes
Sep 5, 2018
5chdn
pushed a commit
that referenced
this pull request
Sep 6, 2018
… unify error handeling for light and full client (#9383) * Provide the actual `account` for eth_coinbase The previous implementation always provided the `zero address` on `eth_coinbase` RPC. Now, instead the actual address is returned on success or an error when no account(s) is found! * full client `eth_coinbase` return err In the full-client return an error when no account is found instead of returning the `zero address` * Remove needless blocks on single import * Remove needless `static` lifetime on const * Fix `rpc_eth_author` test
5chdn
added a commit
that referenced
this pull request
Sep 10, 2018
* parity-version: bump beta to 2.0.4 * [light/jsonrpc] Provide the actual account for `eth_coinbase` RPC and unify error handeling for light and full client (#9383) * Provide the actual `account` for eth_coinbase The previous implementation always provided the `zero address` on `eth_coinbase` RPC. Now, instead the actual address is returned on success or an error when no account(s) is found! * full client `eth_coinbase` return err In the full-client return an error when no account is found instead of returning the `zero address` * Remove needless blocks on single import * Remove needless `static` lifetime on const * Fix `rpc_eth_author` test * parity: print correct keys path on startup (#9501) * aura: don't report skipped primaries when empty steps are enabled (#9435) * Only check warp syncing for eth_getWorks (#9484) * Only check warp syncing for eth_getWorks * Use SyncStatus::is_snapshot_syncing * Fix Snapshot restoration failure on Windows (#9491) * Close Blooms DB files before DB restoration * PR Grumbles I * PR Grumble * Grumble
phahulin
added a commit
to poanetwork/eth-net-intelligence-api
that referenced
this pull request
Nov 9, 2018
Newer versions of parity don't return zero address in `eth_coinbase` when no accounts exist (openethereum/parity-ethereum#9383). It prevents netstats agent from obtaining correct node version info.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous implementation always provided the
zero addressoneth_coinbaseRPCs.Now instead, the actual address is returned on
success and an error when no accounts are found!
Note, this behavior is different from the
full clientwhere we return thezero addressif no accounts are found!Manual tests
Account exists
Account doesn't exist