-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
minor: Use height in query account txs #1525
minor: Use height in query account txs #1525
Conversation
6bc10fb
to
64de1dd
Compare
64de1dd
to
61a7135
Compare
Thank you @0xBigBoss for this first contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment and question but no blockers on my end. Massive improvement!
types/indexer.go
Outdated
return []byte(fmt.Sprintf("%s/%s/%s", | ||
TxSignerKey, | ||
signer, | ||
elenEncoder.EncodeInt(int(height)), // totally safe right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// totally safe right?
-> Do you think we'll hit an overflow issue or something? If so, we can just force the user to pass an int into this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh i think this was more of a joke to myself, I don't think we will be still babysitting v0 after 2^63-1 (or 2^31-1 on 32bit systems) blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the joke with a real comment (so future readers aren't confused as much as I do appreciate it), but otherwise lgtm
@@ -200,19 +211,33 @@ func (t *TransactionIndexer) heightQuery(condition query.Condition, pagination * | |||
return t.getByPrefix(prefixKeyForHeight(height), pagination) | |||
} | |||
|
|||
func (t *TransactionIndexer) signerQuery(condition query.Condition, pagination *query.Page) (res []*types.TxResult, total int, err error) { | |||
signer, err := hex.DecodeString(condition.Operand.(string)) | |||
func (t *TransactionIndexer) signerQuery(primaryCondition query.Condition, secondaryCondition query.Condition, pagination *query.Page) (res []*types.TxResult, total int, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not actionable: I don't think we'll need any more features here, otherwise I would've suggested variadic params (for v1 stuff).
Co-authored-by: Daniel Olshansky <[email protected]>
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information |
AI-Generated Pull Request Summary: This pull request contains 2 commits that introduce the use of block height in the query account transactions command. It adds functionality to search for account transactions with a specific height, useful for filtering transactions. Additionally, this PR includes some context to clarify the behavior of the height parameter when equal to 0. |
AI-Generated Pull Request Summary: This pull request contains a series of 3 patches:
Overall, these patches improve the functionality and maintainability of the codebase. |
// Transaction(tx) Block height index key. | ||
// | ||
// Note: Block height is cast as int when querying using the tx height index due to the lexicographic encoder library, | ||
// this is safe because v0 block height should never exceed 2^63-1 on 64-bit systems or 2^31-1 on 32-bit systems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT (not actually): Block height should never exceed 2^31-1
on any 32+ bit system ;)
This PR Adds a new query parameter to the accounttxs endpoint to allow querying for account (send/received) txs starting at a certain height.
This is not a breaking change, as the new parameter is optional. If it is not provided, the endpoint will behave as it did before.
It is a performance improvement, as it allows the client to specify a height to start querying from, instead of having to query all txs and then filter them out.
You can try and verify by running the following commands:
CURRENT_POKT_RPC_URL= # replace with current rpc url
POKT_RPC_WITH_HEIGHT_INDEX_URL= # replace with new rpc url built from this branch
Tested using the SendNodes POPs address.
pull hash and height of first and last tx
query new version with last height
double check old version still matches