Skip to content
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

Add amount_by_sender query to storage to return transaction amounts and denominations grouped by sender address #3098

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

aubrika
Copy link
Contributor

@aubrika aubrika commented Sep 25, 2023

Closes #3068

@aubrika aubrika requested a review from cronokirby September 25, 2023 23:01
@aubrika aubrika temporarily deployed to smoke-test September 25, 2023 23:01 — with GitHub Actions Inactive
Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

I think this is what we want, although I'm not sure if the GROUP BY in the query does what we want it to do. I think the code should work without it.

JOIN notes ON spendable_notes.note_commitment = notes.note_commitment
JOIN assets ON assets.asset_id = notes.asset_id
WHERE block_height BETWEEN ?1 AND ?2
GROUP BY return_address";
Copy link
Contributor

Choose a reason for hiding this comment

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

What does GROUP BY do for Blobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without grouping by the address, you won't filter out asset amounts for the other addresses, because the JOIN does not otherwise take this into account/is not unique to the address. I initially thought the same but testing revealed the need to specify a specific grouping to return the appropriate values per address.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we tested the case where you send the exact same amount twice? I think the GROUP BY might clobber that

Copy link
Contributor

Choose a reason for hiding this comment

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

Like my understanding of how GROUP BY works without aggregation functions is that it's like doing SELECT DISTINCT, which would have this undesired behavior when you see a repeat of (return_addr, denom, amount)

pub async fn amount_by_sender(
&self,
start_height: Option<u64>,
end_height: Option<u64>,
) -> anyhow::Result<BTreeMap<(Address, String), u128>> {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want this to do the summarization internally? Wouldn't it be more flexible to have something like

notes_by_sender(&self, return_address: &Address) -> Result<Vec<SpendableNoteRecord>>

and then let the caller figure out how to interpret the results?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we want the signature to be such that the output contains all the addresses, rather than us having to figure out what addresses we need to pass into the function. This would be a return type of like Iterator<(Address, SpendableNoteRecord)> with the address appearing multiple times.

While this might be useful in more situations, right now the single consumer of this API just wants the total amount received. I think we can just punt the issue of providing a more generally useful version of the API once there's more consumers of that API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay actually I think implementing Henry's suggestion makes sense now :p

Copy link
Member

Choose a reason for hiding this comment

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

The key point is that for the coordinator we're making this API for, we only ever want to query for single, specific addresses, and we don't want to have to scan through a potentially unbounded amount of data to run the query.

@aubrika aubrika temporarily deployed to smoke-test September 27, 2023 21:50 — with GitHub Actions Inactive
Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@cronokirby cronokirby merged commit 93ae23d into main Sep 27, 2023
8 checks passed
@cronokirby cronokirby deleted the tx-by-amount-3 branch September 27, 2023 22:11
@aubrika aubrika modified the milestone: Testnet 62: Iapetus Oct 4, 2023
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.

Storage Method for Recipient Tallying
3 participants