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.

Sign up for GitHub

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

Jump to bottom

feat(offchain): adds the authority-claimer #45

Merged
merged 1 commit into from
Aug 10, 2023
Merged

feat(offchain): adds the authority-claimer #45

merged 1 commit into from
Aug 10, 2023

Conversation

renan061
Copy link
Contributor

@renan061 renan061 commented Jul 31, 2023 •
edited
Loading

This is mostly code imported from the dispatcher.

Had to do some code massaging to fit Rust's restrictions when using traits.

@renan061 renan061 self-assigned this Jul 31, 2023
This was linked to issues Jul 31, 2023
Create the new authority-claimer service skeleton #34
Closed
Add health-check and metrics to authority-claimer #35
Closed
GCdePaula
GCdePaula reviewed Jul 31, 2023
View reviewed changes
Copy link
Contributor

@GCdePaula GCdePaula left a comment

Choose a reason for hiding this comment

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

I see we're not reading the state of the blockchain to send claims. Are you absolutely sure we don't need it anymore?

offchain/authority-claimer/src/claimer.rs Outdated
Comment on lines 22 to 23
async fn start(
&'a self,
broker_listener: L,
claim_sender: S,
) -> Result<(), AuthorityClaimerError<S, L>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This construction is curious to me, what's the self for? Couldn't this be just a normal function, and the type parameter be part of the function? Or Rust's impl feature (e.g. broker_listener: impl BrokerListener)?

Copy link
Contributor Author

@renan061 renan061 Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You are a lifesaver, I fought for days with Rust and hated this solution.
I just needed to change stuff to the function...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hahaha, no problem! But don't forget the top review comment on not reading the state of the blockchain anymore. That's the most important.

@renan061
Copy link
Contributor Author

renan061 commented Jul 31, 2023 •
edited
Loading

I see we're not reading the state of the blockchain to send claims. Are you absolutely sure we don't need it anymore?

Not absolutely.
But we read from the blockchain to get the number of claims sent.
Can't we get that from the broker?

@GCdePaula
Copy link
Contributor

GCdePaula commented Jul 31, 2023 •
edited
Loading

Are the blockchain and the broker listener certainly in sync? Is there any chance the authority may send duplicated claims?

@renan061
Copy link
Contributor Author

Are the blockchain and the broker listener certainly in sync? Is there any chance the authority may send duplicated claims?

That is what I'm going to start to think long and hard about. Hehe.

It may be we need to read from the blockchain for certainty.
Maybe we don't.
And maybe we could, but there is a lighter way to synchronize stuff.

@GCdePaula
Copy link
Contributor

GCdePaula commented Jul 31, 2023 •
edited
Loading

Yeah, I think we'll need to haha. What happens if the node or redis crashes, or get ejected, or whatever?

@gligneul
Copy link
Contributor

I agree with @GCdePaula. The claimer should read from the blockchain which claims were already sent.

gligneul
gligneul suggested changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

The code looks great. The main thing missing is the trait to get whether the claim is already in the blockchain.

offchain/indexer/Cargo.toml Outdated Show resolved Hide resolved
offchain/authority-claimer/src/main.rs Show resolved Hide resolved
offchain/authority-claimer/src/claimer.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/claimer.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/claimer.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/mod.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/mod.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/mod.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/listener.rs Outdated Show resolved Hide resolved
@renan061 renan061 force-pushed the feature/authority-claimer branch 3 times, most recently from 1209758 to be50d3f Compare August 7, 2023 16:49
gligneul
gligneul reviewed Aug 7, 2023
View reviewed changes
offchain/authority-claimer/src/checker.rs Show resolved Hide resolved
offchain/Cargo.toml Show resolved Hide resolved
offchain/authority-claimer/src/claimer.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/claimer.rs Show resolved Hide resolved
offchain/authority-claimer/src/config/error.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/error.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/error.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/json.rs Show resolved Hide resolved
offchain/authority-claimer/src/config/mod.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/mod.rs Outdated Show resolved Hide resolved
gligneul
gligneul suggested changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@gligneul gligneul left a comment

Choose a reason for hiding this comment

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

This is looking pretty good! Just minor comments.

gligneul
gligneul previously approved these changes Aug 8, 2023
View reviewed changes
GMKrieger
GMKrieger previously approved these changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@GMKrieger GMKrieger left a comment

Choose a reason for hiding this comment

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

Looks awesome!

torives
torives reviewed Aug 8, 2023
View reviewed changes
offchain/authority-claimer/src/claimer.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/cli.rs Show resolved Hide resolved
offchain/authority-claimer/src/config/cli.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/config/cli.rs Show resolved Hide resolved
offchain/authority-claimer/src/lib.rs Outdated Show resolved Hide resolved
offchain/authority-claimer/src/listener.rs Show resolved Hide resolved
@renan061 renan061 dismissed stale reviews from GMKrieger and gligneul via 87e5284 August 9, 2023 15:45
@renan061 renan061 merged commit 2c552ae into next Aug 10, 2023
15 checks passed
@renan061 renan061 deleted the feature/authority-claimer branch August 10, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torives torives torives approved these changes

@GCdePaula GCdePaula GCdePaula left review comments

@gligneul gligneul gligneul approved these changes

@GMKrieger GMKrieger GMKrieger left review comments

Assignees

@renan061 renan061

Labels
None yet
Projects
Node Unit
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add health-check and metrics to authority-claimer Create the new authority-claimer service skeleton
5 participants
@renan061 @GCdePaula @gligneul @torives @GMKrieger