add withKeys option on account #6123
Conversation
facade/nodeFacade.go
Outdated
|
|
||
| for _, address := range addresses { | ||
| accountResponse, blockInfoForAccount, err := nf.node.GetAccount(address, options) | ||
| accountResponse, blockInfoForAccount, err := nf.node.GetAccount(address, options, ctx) |
There was a problem hiding this comment.
I think we have an old issue here
seems that here we could return accounts with different block info, but in the end we reply with the latest block info, which might not be correct.
Could you request with the same roothash for all accounts?
There was a problem hiding this comment.
yes, changed the options so that all accounts are requested with the same roothash, from the first account
facade/nodeFacade.go
Outdated
| @@ -336,7 +339,10 @@ func (nf *nodeFacade) ComputeTransactionGasLimit(tx *transaction.Transaction) (* | |||
|
|
|||
| // GetAccount returns a response containing information about the account correlated with provided address | |||
| func (nf *nodeFacade) GetAccount(address string, options apiData.AccountQueryOptions) (apiData.AccountResponse, apiData.BlockInfo, error) { | |||
| accountResponse, blockInfo, err := nf.node.GetAccount(address, options) | |||
| ctx, cancel := nf.getContextForApiTrieRangeOperations() | |||
There was a problem hiding this comment.
could we do the getContextForApiTrieRangeOperations only if withKeys option is set?
There was a problem hiding this comment.
Right.
Couldn't we just have a GetAccountWithKeys rather(if withKeys) so that we do not change the existing node and only use that if that bool flag is found?
There was a problem hiding this comment.
good observation: refactored to have GetAccountWithKeys
facade/nodeFacade.go
Outdated
| @@ -321,7 +321,10 @@ func (nf *nodeFacade) GetLastPoolNonceForSender(sender string) (uint64, error) { | |||
|
|
|||
| // GetTransactionsPoolNonceGapsForSender will return the nonce gaps from pool for sender, if exists, that is to be returned on API calls | |||
| func (nf *nodeFacade) GetTransactionsPoolNonceGapsForSender(sender string) (*common.TransactionsPoolNonceGapsForSenderApiResponse, error) { | |||
| accountResponse, _, err := nf.node.GetAccount(sender, apiData.AccountQueryOptions{}) | |||
| ctx, cancel := nf.getContextForApiTrieRangeOperations() | |||
There was a problem hiding this comment.
I think here we can remove the getContextForApiTrieRangeOperations as this call does not support options.
There was a problem hiding this comment.
refactored to have a method GetAccountWithKeys
facade/nodeFacade.go
Outdated
| @@ -336,7 +339,10 @@ func (nf *nodeFacade) ComputeTransactionGasLimit(tx *transaction.Transaction) (* | |||
|
|
|||
| // GetAccount returns a response containing information about the account correlated with provided address | |||
| func (nf *nodeFacade) GetAccount(address string, options apiData.AccountQueryOptions) (apiData.AccountResponse, apiData.BlockInfo, error) { | |||
| accountResponse, blockInfo, err := nf.node.GetAccount(address, options) | |||
| ctx, cancel := nf.getContextForApiTrieRangeOperations() | |||
There was a problem hiding this comment.
Right.
Couldn't we just have a GetAccountWithKeys rather(if withKeys) so that we do not change the existing node and only use that if that bool flag is found?
node/node_test.go
Outdated
| node.WithStateComponents(stateComponents), | ||
| ) | ||
|
|
||
| recovAccnt, _, err := n.GetAccount(testscommon.TestAddressBob, api.AccountQueryOptions{WithKeys: false}, context.Background()) |
There was a problem hiding this comment.
Would it make sens to test the Getters in parallel here on different go routines?
There was a problem hiding this comment.
do you mean to try to have multiple requests and check the result from different blocks?
There was a problem hiding this comment.
Yes, or from the same block, just making parallel requests.
If it doesn't make sense or it's already being tested somewhere else, we can skip it
There was a problem hiding this comment.
Normal allin test: v1.6.15-dev-config-eb2e06c06d -> MX-15381-add-withKeys-on-a-74c90c5230
--- Specific errors ---
block hash does not match 1725
wrong nonce in block 679
miniblocks does not match 0
num miniblocks does not match 0
miniblock hash does not match 0
block bodies does not match 0
receipts hash missmatch 0
/------/
--- Statistics ---
Nr. of all ERRORS: 2
Nr. of all WARNS: 498
Nr. of new ERRORS: 1
Nr. of new WARNS: 5
Nr. of PANICS: 0
/------/
--- ERRORS ---
/------/
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?