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

feat: record multisig approvals #389

Merged
merged 27 commits into from
Feb 18, 2021
Merged

feat: record multisig approvals #389

merged 27 commits into from
Feb 18, 2021

Conversation

iand
Copy link
Contributor

@iand iand commented Feb 17, 2021

Create and populate a new multisig_approvals table containing multisig transaction approvals derived from propose and approve messages sent to the multisig actor. Create a new task called msapprovals to run the extraction.

Refactored the message task and moved it under the tasks package and then created a msapprovals package as a sibling. Ideally I would like to see all the named tasks move in this way so the chain package just contains chain walking/watching logic.

Anecdotally the msapprovals task takes a few seconds to run per tipset, depending on the number of multisig messages.

(apologies for the dozens of hacky commits which I needed for shipping code for intractive testing - will squash them anyway before merge)

Fixes #388

@codecov-io
Copy link

Codecov Report

Merging #389 (71ba839) into master (11d0ef1) will increase coverage by 3.5%.
The diff coverage is 15.3%.

@@           Coverage Diff            @@
##           master    #389     +/-   ##
========================================
+ Coverage    42.9%   46.4%   +3.5%     
========================================
  Files          25      24      -1     
  Lines        1940    1796    -144     
========================================
+ Hits          833     835      +2     
+ Misses        980     834    -146     
  Partials      127     127             

@iand iand requested review from frrist and hsanjuan February 17, 2021 17:38
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

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

Left a comment about migrations that should be addressed before merging, otherwise looks good to me 🚢

"to" text not null,
"value" numeric not null,
"signers" jsonb not null,
PRIMARY KEY ("height", "state_root", "multisig_id")
Copy link
Member

Choose a reason for hiding this comment

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

Based on the definition of the MultisigApproval model (which I believe to be correct) Approver and Message should be primary keys here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

default:
}

// ll.Infow("found message", "to", m.ToActorCode.String(), "addr", m.Message.To.String())
Copy link
Member

Choose a reason for hiding this comment

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

✂️

Comment on lines 245 to 250
if err == nil && !found {
return cid.Undef
}
if err != nil {
return cid.Undef
}
Copy link
Member

Choose a reason for hiding this comment

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

if err != nil  || !found {
  return cid.Undef
}

is a tad clearer imo.

@iand iand merged commit 3dfc8c7 into master Feb 18, 2021
@iand iand deleted the iand/multisig-approvals branch February 18, 2021 10:45
@hsanjuan
Copy link
Contributor

Thanks for tackling this @iand and for reviewing @frrist !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Track multisig transactions
4 participants