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

lite2: remove auto update #4535

Merged
merged 2 commits into from
Mar 6, 2020
Merged

lite2: remove auto update #4535

merged 2 commits into from
Mar 6, 2020

Conversation

melekes
Copy link
Contributor

@melekes melekes commented Mar 5, 2020

Description

We first introduced auto-update as a separate struct AutoClient, which
was wrapping Client and calling Update periodically.

// AutoClient can auto update itself by fetching headers every N seconds.
type AutoClient struct {
    base         *Client
    updatePeriod time.Duration
    quit         chan struct{}

    trustedHeaders chan *types.SignedHeader
    errs           chan error
}

// NewAutoClient creates a new client and starts a polling goroutine.
func NewAutoClient(base *Client, updatePeriod time.Duration) *AutoClient {
    c := &AutoClient{
        base:           base,
        updatePeriod:   updatePeriod,
        quit:           make(chan struct{}),
        trustedHeaders: make(chan *types.SignedHeader),
        errs:           make(chan error),
    }
    go c.autoUpdate()
    return c
}

// TrustedHeaders returns a channel onto which new trusted headers are posted.
func (c *AutoClient) TrustedHeaders() <-chan *types.SignedHeader {
    return c.trustedHeaders
}

// Err returns a channel onto which errors are posted.
func (c *AutoClient) Errs() <-chan error {
    return c.errs
}

// Stop stops the client.
func (c *AutoClient) Stop() {
    close(c.quit)
}

func (c *AutoClient) autoUpdate() {
    ticker := time.NewTicker(c.updatePeriod)
    defer ticker.Stop()

    for {
        select {
        case <-ticker.C:
            lastTrustedHeight, err := c.base.LastTrustedHeight()
            if err != nil {
                c.errs <- err
                continue
            }

            if lastTrustedHeight == -1 {
                // no headers yet => wait
                continue
            }

            newTrustedHeader, err := c.base.Update(time.Now())
            if err != nil {
                c.errs <- err
                continue
            }

            if newTrustedHeader != nil {
                 c.trustedHeaders <- newTrustedHeader
            }
        case <-c.quit:
            return
        }
    }
}

Later we merged it into the Client itself with the assumption that most clients will want it.

But now I am not sure. Neither IBC nor cosmos/relayer are using it. It increases complexity (Start/Stop methods).

That said, I think it makes sense to remove it until we see a need for it (until we better understand usage behavior). We can always introduce it later 😅. Maybe in the form of AutoClient.


For contributor use:

  • Wrote tests
  • Updated CHANGELOG_PENDING.md
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments
  • Re-reviewed Files changed in the Github PR explorer

We first introduced auto-update as a separate struct AutoClient, which
was wrapping Client and calling Update periodically. Later, we merged it
into the Client itself.

There are two issues with this feature:

1. We don't know if it will be used (neither IBC nor cosmos/relayer use
   it);
2. It increases complexity (Start/Stop methods).

That said, I think it makes sense to remove it until we see a need for
it.
@melekes melekes requested review from ebuchman and tessr as code owners March 5, 2020 17:50
@melekes melekes requested a review from cmwaters March 5, 2020 17:51
@melekes melekes self-assigned this Mar 5, 2020
@melekes melekes added the C:light Component: Light label Mar 5, 2020

```go
type Client interface {
// start and stop updating & cleaning goroutines
Start() error
Stop()
Cleanup() error
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would swap this with the verification part of the interface. I like to have them in order of importance

Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I think it would be relatively easy to re-implement the autoclient in the future

@melekes melekes merged commit 431618c into master Mar 6, 2020
@melekes melekes deleted the anton/lite2-remove-auto-update branch March 6, 2020 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:light Component: Light
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants