-
Notifications
You must be signed in to change notification settings - Fork 2
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
Updates to Substreams Balance Changes #17
base: master
Are you sure you want to change the base?
Updates to Substreams Balance Changes #17
Conversation
substreams-ethereum = "0.9" | ||
substreams-entity-change = "1.3" | ||
substreams-database-change = "1.3.0" | ||
substreams = "0.6" |
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.
The buf.gen.yaml defined plugins should be updated too. See https://github.com/streamingfast/substreams-foundational-modules/blob/develop/ethereum-common/buf.gen.yaml#L4-L10
@@ -1,1257 +1,1198 @@ | |||
const INTERNAL_ERR: &'static str = "`ethabi_derive` internal 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.
This make me realized that generated code isn't flag with // @generated
which seems to be picked up by cargofmt
and avoid re-formatting the code in those case.
I'll add that to substreams-ethereum
.
// -- storage change -- | ||
string storage_key = 10; | ||
uint64 storage_ordinal = 11; | ||
string storage_address = 12; |
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.
What will those gives exactly in terms of real usage?
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.
storage_address
I understand, for the other 2, now sure it's really worth it.
// -- transfer -- | ||
string from = 25; | ||
string to = 26; | ||
string value = 27; |
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.
Wondering if we should offer those as a separate table output, I'm fearing the oversized of the output data. If there would be 2 tables, transfers & balances they could be linked together.
But the transfers table itself would need metadata, leading to extra metadata gain. So maybe it's not even worth it.
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.
In fact just realized there is an actual table transfer
. So maybe we should just "join" them and remove the data here.
// -- indexing -- | ||
uint64 version = 30; // latest version of the balance change (block_num << 32 + storage_ordinal) |
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 don't like the version
name, I feel it's really miss leading. global_sequence
maybe.
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.
Also, the usage of storage_ordinal
as an identifier raises some fear from my side. Ordinals are good for ordering but as soon as there is a small piece of indeterminism in the tracing, all the ordinals are shifted.
If the ordinals are used only for ordering, in 99% of the case, the end processing will result in the same output even if the ordinals are different. But if they are used as IDs, it contaminates the output.
Could we use the relative ordering of the storage changes instead? That would lead to output differences only if ordering of storage balance changes would be different.
I'll try next week to check further the code and try to provide an alternative.
} | ||
// -- storage change -- | ||
string storage_key = 10; | ||
uint64 storage_ordinal = 11; |
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.
See comment about ordinals on version
field in this file. We should remove this one or maybe replace by our "relative" ordering. But the field version
already acts as that, so outside of debugging, we should I think not add this.
uint32 call_index = 4; | ||
string transfer_value = 5; | ||
// -- debug -- | ||
int32 change_type = 99; // type enum isn't supported yet as a leaf node |
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.
If I add support for this in the Parquet sink, do you know how it would map in the engine? I fear to us named value, is it the string value that would see be stored in the Parquet data?
@@ -1,4 +1,4 @@ | |||
[toolchain] | |||
channel = "1.70" | |||
channel = "1.76" |
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.
You can bump to much higher I tink no?
for call in child_calls.iter() { | ||
storage_changes.extend(call.storage_changes.clone()); | ||
} |
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 is so much .clone()
. Most were there event before.
I'll try to push on top of branch to remove most of them.
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.
pub fn to_balance_change(clock: &Clock, trx: &TransactionTrace, call: &Call, log: &Log, transfer: &TransferAbi, owner: Vec<u8>, storage_change: &StorageChange, change_type: BalanceChangeType) -> BalanceChange { | ||
let old_balance = BigInt::from_unsigned_bytes_be(&storage_change.old_value); | ||
let new_balance = BigInt::from_unsigned_bytes_be(&storage_change.new_value); | ||
let amount = new_balance.clone() - old_balance.clone(); |
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 amount = new_balance.clone() - old_balance.clone(); | |
let amount = &new_balance - &old_balance; |
Done
BalanceChange
protobuf types #11date
field #10version
and/orindex
field to balance changesdb_out
/graph_out
#9from
sender #8Updated to latest Substreams
Remaining