-
Notifications
You must be signed in to change notification settings - Fork 5
refactor: group UTxO and address deltas by tx #358
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
Conversation
Signed-off-by: William Hankins <william@sundae.fi>
Signed-off-by: William Hankins <william@sundae.fi>
Signed-off-by: William Hankins <william@sundae.fi>
Signed-off-by: William Hankins <william@sundae.fi>
Signed-off-by: William Hankins <william@sundae.fi>
| pub received_sum: AmountList, | ||
| pub sent_sum: AmountList, | ||
| pub tx_count: u64, | ||
| pub tx_count: u32, |
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.
I wonder how many centuries Cardano will need to exist for, before an account is affected by a 4,294,967,296th transaction and makes this endpoint fail
| #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] | ||
| pub struct TxTotals { | ||
| pub sent: ValueDelta, | ||
| pub received: Value, | ||
| } |
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.
Why is sent a ValueDelta but received a Value? It looks like in at least a few places, we assume that sent.lovelace is positive.
(also, it might make some code a little cleaner if TxTotals had an as_delta helper method which returned its data as a ValueDelta)
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.
Thanks for catching this Simon. I had assumed ValueDelta was needed downstream but this isn't the case. I've updated TxTotals and AddressTotals to use Value for the sent field in df09d6f.
lowhung
left a comment
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.
Nicely done 👍
|
|
||
| for result in self.tx_count.prefix(account.get_hash().as_ref()) { | ||
| let (_, bytes) = result?; | ||
| let epoch_count = u32::from_le_bytes(bytes[..4].try_into()?); |
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.
French methods 🤣
| store-spdd = false | ||
|
|
||
| [module.historical-accounts-state] | ||
| # Enables /accounts/{stake_address}/rewards endpoint |
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.
Thanks for these!
Signed-off-by: William Hankins <william@sundae.fi>
Description
This PR refactors both
UTXODeltasMessageandAddressDeltasMessageto group deltas per transaction instead of per individual UTxO delta. This fixes incorrect transaction counting in bothaddress_stateandhistorical_accounts_state, particularly in cases where:Account level transaction counting is now handled separately in
historical_accounts_state, avoiding the need to indexTxIdentifiervalues by address. A newGetAccountTotalTxCountquery is added to support the/accounts/{stake_address}/addresses/totalendpoint.Related Issue(s)
How was this tested?
/accounts/{stake_address}/addresses/totalsto confirm that tx counts now match expected behavior.Checklist
Impact / Side effects
utxo_stateconstructs a temporaryAddressTxMapper transaction to aggregate UTxO deltas before publishing address level deltas.StakeDeltasMessagenow includes the per block tx count in the delta.Reviewer notes / Areas to focus
tx_unpacker/src/tx_unpacker.rshandleinutxo_state/src/state.rsprocess_messageinstake_delta_filter/src/utils.rs