Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Optimize safe events indexer #2194
Optimize safe events indexer #2194
Changes from 9 commits
5f3c276
77cea72
2140074
6949ed4
5157e18
7016ad5
7b5081b
044029f
2253970
6730314
2764687
023a98f
d8e0195
5eeabd4
74283bb
0ffb37b
5ece964
efe5fa5
e63fb87
2139ad2
ec803bc
bddf780
7c89e50
bd7fef2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 previous implementation always tried to remove the
InternalTx
generated fromProxyCreation
just in case theProxyCreation
came first. https://github.com/safe-global/safe-transaction-service/blob/main/safe_transaction_service/history/indexers/safe_events_indexer.py#L366In such case, because the
ProxyCreation
never comes later, theInternalTx
generated bySafeSetup
leaves without singleton (to) forever.Maybe would be better try to get the Proxy InternalTx from database to insert the correct parameters of
SafeSetup
what do you think? @safe-global/core-apiIn the current implementation the
ProxyCreation
is storing the singleton address in theto
field ofInternalTx
, not sure if is completely 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.
What happens if they are already inserted?
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.
Raise an exception.
Ignoring conflicts in such case would fix the issue, 5eeabd4
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.
@Uxio0
ignore_conflicts
fail becauseInternalDecodedTx
have relation withinternal_tx
, basically the ORM try to prevent lose data.We can use instead of that update_conflicts, but I'm concerned because I just discovered that bulk_create does not call save, and in that case
post_save
will not be fired.Will this affect our signals handle?
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.
bulk_create
does in case we inherit our BulkCreateMixinThere 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 did a workaround in case that would be a collision inserting the same instance, shouldn't be any common because the bulk create is in atomic section and we are checking if the contract were indexed, basically if the
setup
andproxyevent
were correctly inserted, shouldn't reach the insert section if would be indexed again.