Add and fix missing txn/staking txn rpc apis #2343
Conversation
denniswon
requested review from
rlan35,
fxfactorial,
LeoHChen,
flicker-harmony and
a team
node/node_explorer.go
Outdated
| return addressData.TXs[i].Timestamp < addressData.TXs[j].Timestamp | ||
| }) | ||
| } | ||
| hashes := make([]common.Hash, 0) |
There was a problem hiding this comment.
prefer literal when don't have dynamic size, aka hashes := []common.Hash{}
hmy/api_backend.go
Outdated
| @@ -241,6 +240,12 @@ func (b *APIBackend) GetTransactionsHistory(address, txType, order string) ([]co | |||
| return hashes, err | |||
| } | |||
|
|
|||
| // GetStakingTransactionsHistory returns list of staking transactions hashes of address. | |||
| func (b *APIBackend) GetStakingTransactionsHistory(address, txType, order string) ([]common.Hash, error) { | |||
| hashes, err := b.hmy.nodeAPI.GetStakingTransactionsHistory(address, txType, order) | |||
There was a problem hiding this comment.
might as well just return b.hmy.... the signatures are the same
There was a problem hiding this comment.
could you clarify your comment here?
There was a problem hiding this comment.
the signature of b.hmy.nodeAPI.GetStakingTransactionsHistory is the same as the RPC GetStakingTransactionsHistory so no point to hold onto intermediate variables
There was a problem hiding this comment.
i see. so no possibility they will/could become different between regular vs. staking txns, correct?
There was a problem hiding this comment.
of course its possible, its an open source project, anything can change. But we can change when needed, rather than adds lines of code now.
node/node_explorer.go
Outdated
|
|
||
| // GetStakingTransactionsHistory returns list of staking transactions hashes of address. | ||
| func (node *Node) GetStakingTransactionsHistory(address, txType, order string) ([]common.Hash, error) { | ||
| addressData := &explorer.Address{} |
There was a problem hiding this comment.
If the code for this function is exactly same as GetTransactionsHistory, can't you just call it?
There was a problem hiding this comment.
yes, no need for even further code duplication
There was a problem hiding this comment.
Overall, the guys already commented on most things. But I have separate PR which might get in conflict with your code. #2317
|
Jenkins, test this please. |
denniswon
force-pushed
the
pr_txn_history
branch
2 times, most recently
from
a8709de to
0440a60
Compare
denniswon
force-pushed
the
pr_txn_history
branch
from
0440a60 to
6ce1733
Compare
|
@flicker-harmony this PR fixes the open staking launch blocker issue #2227. @fxfactorial @gupadhyaya please review. |
denniswon
added
block-explorer-backend
|
From @flicker-harmony told me about there was an issue previously that might be why I am getting the error during my local testing where the explorer node's level db does not get updated even with account txns. And this issue on master is related to the staking changes in node/node_explorer.go where quorum gets checked? (It fails and so no blocks are committed to explorer storage) @rlan35 @fxfactorial any context? so should I rebase to s3? cc: @LeoHChen which branch is being pushed to ostn? s3 or t3? This issue is what above refers to: #2266 |
* Add and fix missing txn/staking txn rpc apis * removed StakingTxHistoryArgs as currently not needed yet
…" (harmony-one#2369) This reverts commit 84abf83.
Issue
Fixes issue #2227
Explorer for staking network is not displaying the transaction history
Test
Unit Test Coverage
Before:
After:
Test/Run Logs
Operational Checklist
Does this PR introduce backward-incompatible changes to the on-disk data structure and/or the over-the-wire protocol?. (If no, skip to question 8.)
NO
Does this PR introduce backward-incompatible changes NOT related to on-disk data structure and/or over-the-wire protocol? (If no, continue to question 11.)
NO. (There is no change in API name or args. Just addition of new api funcs)
Does the existing
node.shcontinue to work with this change?YES
What should node operators know about this change?
Harmony doc should be updated with the new api functions with explanations.
Also, fixed some api function comments correcting the functionality/purpose of the function.
cc: @Renewwen @Cem-Harmony could you help me update these once merged and released?
Does this PR introduce significant changes to the operational requirements of the node software, such as >20% increase in CPU, memory, and/or disk usage?
NO
TODO