-
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
Multisig parser #196
Multisig parser #196
Conversation
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.
Code looks pretty great. But we need to add some tests here. Now that this is wrapped, we can add tests as we have for other parsing functions.
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 like the approach. But we need to test this at least in a few cases. Set a bunch of block cids, generate the tipset key and compare with the expected 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.
Just one last comment. It looks great!
@@ -180,12 +180,12 @@ func (h *Helper) FilterTxsByActorType(ctx context.Context, txs []*types.Transact | |||
addrTo, err := address.NewFromString(tx.TxTo) | |||
if err != nil { | |||
h.logger.Sugar().Errorf("could not parse address. Err: %s", err) | |||
return nil, err |
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 really like this, as if something fails, we loose the tx at all, and never know. I guess this is the general approach on the fil parser, so I am ok now. But in the future we need a way to track these errors, as we won't never be able to fix them otherwise.
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.
agree, we need to include some metrics here
@@ -87,7 +87,7 @@ func (eg *eventGenerator) GenerateMultisigEvents(ctx context.Context, transactio | |||
addrTo, err := address.NewFromString(tx.TxTo) | |||
if err != nil { | |||
eg.logger.Sugar().Errorf("could not parse address. Err: %s", err) | |||
return nil, err |
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.
Same happens here.
🔗 zboto Link