celestiaorg / go-header

Go library with all the services needed to request, sync and store blockchain headers.
Apache License 2.0
17 stars 16 forks source link

misc(syncer): make timeout configurable #162

Closed AryanGodara closed 3 months ago

AryanGodara commented 3 months ago

Overview

Resolves #155

Checklist

AryanGodara commented 3 months ago

@vgonkivs This one's ready for review :)

AryanGodara commented 3 months ago

This is wrong. The State struct is not supplied to the Syncer, but the Syncer provides it's cuttent state to user through State method on Syncer. Therefore, you can't configure anything through it in Syncer.

Instead, we should add this configuration field to the Config

I see. I'm sorry for this error, I'll make the changes asap!

vgonkivs commented 3 months ago

hey @AryanGodara, thanks for you contribution. Let me help you to improve your code: If you take a look here, you will see the Parameter struct that allows configuring the syncer. In the syncer c-tor we create default parameters and override them using options pattern, which you can find in the same files I've added previously. The idea is to add maxAwait to the Parameter struct + set the default value in DefaultParameters and add the ability to override the default maxAwait time.

AryanGodara commented 3 months ago

hey @AryanGodara, thanks for you contribution. Let me help you to improve your code: If you take a look here, you will see the Parameter struct that allows configuring the syncer. In the syncer c-tor we create default parameters and override them using options pattern, which you can find in the same files I've added previously. The idea is to add maxAwait to the Parameter struct + set the default value in DefaultParameters and add the ability to override the default maxAwait time.

Wow thank you for explaining it in such detail @vgonkivs :D This was extremely helpful. I've just pushed new changes. I hope this time, I've implemented it correctly (:

renaynay commented 3 months ago

Hi @AryanGodara thank you so much for your contribution. We've decided that we don't actually need this feature unfortunately. I apologise for the last minute change of mind. There are many other good first issues that we welcome you to work on, however!

AryanGodara commented 3 months ago

Hi @AryanGodara thank you so much for your contribution. We've decided that we don't actually need this feature unfortunately. I apologise for the last minute change of mind. There are many other good first issues that we welcome you to work on, however!

It's quite alright @renaynay :D It was a good issue to get onboarded to the project. I hope I'll continue making new good contributions over time :D