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

Implement bottom up validation #398

Merged
merged 8 commits into from
Feb 2, 2021
Merged

Conversation

akhilkumarpilli
Copy link
Contributor

Fixes: #345

@akhilkumarpilli akhilkumarpilli marked this pull request as ready for review January 30, 2021 05:29
cmd/config.go Outdated Show resolved Hide resolved
cmd/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

@colin-axner colin-axner requested a review from anilcse February 2, 2021 11:12
Copy link
Collaborator

@anilcse anilcse left a comment

Choose a reason for hiding this comment

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

lgtm, just small nits

cmd/config.go Outdated
Comment on lines 425 to 430
if err = c.ValidatePathEnd(p.Src); err != nil {
return err
}
if p.Src.Version == "" {
return fmt.Errorf("source must specify a version")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's re-order this check

Suggested change
if err = c.ValidatePathEnd(p.Src); err != nil {
return err
}
if p.Src.Version == "" {
return fmt.Errorf("source must specify a version")
}
if p.Src.Version == "" {
return fmt.Errorf("source must specify a version")
}
if err = c.ValidatePathEnd(p.Src); err != nil {
return err
}

cmd/config.go Outdated
Comment on lines 460 to 474
if pe.ClientID != "" {
if err := c.ValidateClient(chain, height, pe); err != nil {
return err
}
}
if pe.ConnectionID != "" {
if err := c.ValidateConnection(chain, height, pe); err != nil {
return err
}
}
if pe.ChannelID != "" {
if err := c.ValidateChannel(chain, height, pe); err != nil {
return err
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if the ClientID is nil, there's no point in validating connection or channel details. We can just return err if the ClientID is nil and connectionID is not nil. Similarly for ChannelID

Suggested change
if pe.ClientID != "" {
if err := c.ValidateClient(chain, height, pe); err != nil {
return err
}
}
if pe.ConnectionID != "" {
if err := c.ValidateConnection(chain, height, pe); err != nil {
return err
}
}
if pe.ChannelID != "" {
if err := c.ValidateChannel(chain, height, pe); err != nil {
return err
}
}
if pe.ClientID != "" {
if err := c.ValidateClient(chain, height, pe); err != nil {
return err
}
if pe.ConnectionID != "" {
if err := c.ValidateConnection(chain, height, pe); err != nil {
return err
}
if pe.ChannelID != "" {
if err := c.ValidateChannel(chain, height, pe); err != nil {
return err
}
}
}
}
if pe.ClientID == "" && pe.ConnectionID != "" {
return fmt.Errorf("ClientID is not conifgured for the connection: %s", pe.ConnectionID)
}

@anilcse anilcse merged commit 1e0ef8a into master Feb 2, 2021
@anilcse anilcse deleted the akhil/345-bottom-up-validation branch February 2, 2021 13:57
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.

Implement bottom up path validation
3 participants