Skip to content
This repository has been archived by the owner on Oct 31, 2024. It is now read-only.

Address review concerns from PR 39 #43

Merged

Conversation

dvdplm
Copy link
Contributor

@dvdplm dvdplm commented Apr 11, 2024

Follow-up to #39

  • Updated with latest main
  • Address review concerns wrt docs

dvdplm and others added 10 commits March 25, 2024 13:43
fix: idiomatic name for getters
Make signers a private member of AggregatorState
Make state a private member on SignatureAggregator
Prefer returning Result to asserts
Co-authored-by: Hamy Ratoanina <[email protected]>
change: move `dedup_signers` inside `include_signers`
change: use `signers()` everywhere
change: add a `TooManySigners` error
@dvdplm dvdplm requested a review from a team as a code owner April 11, 2024 13:12
@dvdplm dvdplm requested review from 4l0n50 and Nashtare April 11, 2024 13:12
@dvdplm dvdplm mentioned this pull request Apr 11, 2024
Copy link
Member

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

Thank you David! Just a tiny nit regarding a sentence, otherwise LGTM!

src/sign/signature.rs Outdated Show resolved Hide resolved
Copy link
Member

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

LGTM!

@dvdplm
Copy link
Contributor Author

dvdplm commented Apr 19, 2024

@Nashtare Not sure what CI is angry about tbh. :/

@Nashtare
Copy link
Member

Yeah I've no idea either. I ran all jobs locally so I'll merge as is, and advise if it happens again.

@Nashtare Nashtare merged commit 134b5cd into topos-protocol:main Apr 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants