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

New completion worker model #188

Draft
wants to merge 32 commits into
base: master
Choose a base branch
from
Draft

Conversation

ricardomgoncalves
Copy link
Contributor

WIP

@ricardomgoncalves ricardomgoncalves self-assigned this Aug 2, 2022
@ricardomgoncalves ricardomgoncalves changed the base branch from master to currencies August 2, 2022 13:16
@ricardomgoncalves ricardomgoncalves requested review from Ullaakut and awfm9 and removed request for Ullaakut August 4, 2022 15:55
@ricardomgoncalves ricardomgoncalves marked this pull request as ready for review August 4, 2022 15:55
@@ -18,7 +18,7 @@ type Transfer struct {
EventIndex uint `json:"event_index"`
SenderAddress string `json:"sender_address"`
ReceiverAddress string `json:"receiver_address"`
TokenCount uint `json:"token_count"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change necessary?


value, ok := fields["value"].(*big.Int)
if !ok {
return nil, fmt.Errorf("invalid type for \"value\" field (%T)", fields["value"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("invalid type for \"value\" field (%T)", fields["value"])
return nil, fmt.Errorf("invalid type for %q field (%T)", fieldValue, fields[fieldValue])

service/workers/completion_handler.go Outdated Show resolved Hide resolved
Comment on lines +114 to +142
if len(log.Topics) < 3 && len(log.Topics) > 4 {
p.log.Warn().
Uint("index", log.Index).
Int("topics", len(log.Topics)).
Msg("skipping log with invalid topic length")
continue
}

transfer, err := parsers.ERC721Transfer(log)
if err != nil {
return nil, fmt.Errorf("could not parse sale ERC721 transfer: %w", err)
}
switch {

case len(log.Topics) == 3:

transfer, err := parsers.ERC20Transfer(log)
if err != nil {
return nil, fmt.Errorf("could not parse sale ERC20 transfer: %w", err)
}

transferLookup[transfer.Hash()] = append(transferLookup[transfer.Hash()], transfer)
transferLookup[transfer.TransactionHash] = append(transferLookup[transfer.TransactionHash], transfer)

case len(log.Topics) == 4:

transfer, err := parsers.ERC721Transfer(log)
if err != nil {
return nil, fmt.Errorf("could not parse sale ERC721 transfer: %w", err)
}

transferLookup[transfer.TransactionHash] = append(transferLookup[transfer.TransactionHash], transfer)

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't this change already in an earlier PR as well? Does this PR include older commits that weren't merged or is my memory playing tricks on me? 😄

service/workers/completion_handler.go Outdated Show resolved Hide resolved
Comment on lines +214 to +218
// if there is more than 1 nft transfer and 1 token transfer
// (could be 2 nft transfers next cases will cover this)
if len(transfers) > 2 {
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a normal case that there are more than two transfers? When can it happen? How do we deal with it? It seems like the only supported case is exactly 2 transfers, and that other cases trigger warnings

Copy link
Contributor

@awfm9 awfm9 left a comment

Choose a reason for hiding this comment

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

I will take a look at changing the approach this weekend, see comment. This is too brittle, and too hard to keep extending.

config/params/symbols.go Outdated Show resolved Hide resolved
docker-compose.yaml Outdated Show resolved Hide resolved
docker-compose.yaml Outdated Show resolved Hide resolved
service/workers/completion_handler.go Outdated Show resolved Hide resolved
service/workers/completion_handler.go Outdated Show resolved Hide resolved
sql/07_insert_collections_data.sql Outdated Show resolved Hide resolved
sql/07_insert_collections_data.sql Outdated Show resolved Hide resolved
Comment on lines +162 to +168
// Sort everything by log index
sort.Slice(sales, func(i, j int) bool {
return sales[i].EventIndex < sales[i].EventIndex
})
sort.Slice(transactionTransfers, func(i, j int) bool {
return transactionTransfers[i].EventIndex < transactionTransfers[i].EventIndex
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this approach is flawed, as we might end up mixing things. What I would like to see is the ability to get all the events for the transaction, and then pattern-matching a certain "anatomy" for it. We should run all transaction logs through a parser, that matches all the events in the transaction by type to identify what is happening.

@awfm9 awfm9 changed the base branch from currencies to master August 7, 2022 16:54
@awfm9 awfm9 marked this pull request as draft August 23, 2022 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants