Fix config add-dir regression #406

akhilkumarpilli · 2021-02-04T06:47:54Z

Fixes: #401

Splits the add-dir comman into add-chains and add-paths. Adding chain configuration files and adding a key is required before running add-paths since add-paths will query the chains to ensure the path is valid

akhilkumarpilli · 2021-02-04T06:55:00Z

@colin-axner , we are actually getting nil pointer dereference issue from here. Since key is not added for chain, we are unable to get address of it and that's the reason, we are getting error. But we need chain to validate provided identifiers. Can you suggest any solution for this?

colin-axner · 2021-02-04T10:34:19Z

@akhilkumarpilli interesting. I would have thought the key is being set when the chain is being set (which I assumed occurs in the for loop over files). I will take a closer look in a second.

Ideally we set the chains first then the paths. In this fix though I was hoping to set both the chains and the paths and then do validation.

colin-axner · 2021-02-04T10:43:29Z

@akhilkumarpilli are you still getting the error? I tried reproducing @michaelfig's error (thanks for the how-to-reproduce steps) and everything works for me

colin-axner · 2021-02-04T10:44:25Z

cmd/config.go

@@ -263,6 +259,12 @@ func cfgFilesAdd(dir string) (cfg *Config, err error) {
 		}
 		fmt.Printf("added chain %s...\n", c.ChainID)
 	}
+	for _, p := range cfg.Paths {
+		if err = config.ValidatePath(p); err != nil {
+			fmt.Println("Error occured...Config is not updated")


Suggested change

fmt.Println("Error occured...Config is not updated")

maybe we could wrap the error and say the path validation failed

Yeah Sure, but we are displaying added chain logs before. That's the reason, I wrote Config is not updated

akhilkumarpilli · 2021-02-04T10:46:17Z

@akhilkumarpilli are you still getting the error? I tried reproducing @michaelfig's error (thanks for the how-to-reproduce steps) and everything works for me

I am still getting issue. Can you try once by clearing config?

colin-axner

can you update, Skipping non chain file... to say added path: %s\n", pathName

colin-axner · 2021-02-04T10:47:29Z

are you getting this?

panic: assignment to entry in nil map

goroutine 1 [running]:
github.com/cosmos/relayer/relayer.Paths.Add(...)
	/home/bartleby/work/go/src/github.com/cosmos/relayer/relayer/path.go:56
github.com/cosmos/relayer/cmd.(*Config).AddPath(0xc000f96480, 0xc000f7f628, 0x6, 0xc000f87820, 0x0, 0x1)
	/home/bartleby/work/go/src/github.com/cosmos/relayer/cmd/config.go:318 +0x125
github.com/cosmos/relayer/cmd.cfgFilesAdd(0x7ffcf6582a54, 0x7, 0xc000f5fd48, 0x0, 0x0)
	/home/bartleby/work/go/src/github.com/cosmos/relayer/cmd/config.go:221 +0x7d6
github.com/cosmos/relayer/cmd.configAddDirCmd.func1(0xc000dd3600, 0xc000fa4c90, 0x1, 0x1, 0x0, 0x0)
	/home/bartleby/work/go/src/github.com/cosmos/relayer/cmd/config.go:163 +0x45
github.com/spf13/cobra.(*Command).execute(0xc000dd3600, 0xc000fa4c70, 0x1, 0x1, 0xc000dd3600, 0xc000fa4c70)
	/home/bartleby/work/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:850 +0x47c
github.com/spf13/cobra.(*Command).ExecuteC(0x2276b20, 0x0, 0x0, 0x0)
	/home/bartleby/work/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:958 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
	/home/bartleby/work/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:895
github.com/cosmos/relayer/cmd.Execute()
	/home/bartleby/work/go/src/github.com/cosmos/relayer/cmd/root.go:107 +0x4d
main.main()
	/home/bartleby/work/go/src/github.com/cosmos/relayer/main.go:21 +0x25

akhilkumarpilli · 2021-02-04T10:49:57Z

I am getting this:

$ rly config add-dir configs/gaia
Skipping non chain file: demo.json
added chain ibc-0...
added chain ibc-1...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x12ebcf7]

goroutine 1 [running]:
github.com/cosmos/relayer/relayer.(*Chain).GetAddress(0xc000d9be60, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/vitwit/go/src/github.com/cosmos/relayer/relayer/chain.go:422 +0x97
github.com/cosmos/relayer/relayer.Chains.Get(0xc000db3020, 0x2, 0x2, 0xc000d8dde0, 0x5, 0xc00016c000, 0x0, 0xc000c5fac8)
        /home/vitwit/go/src/github.com/cosmos/relayer/relayer/chain.go:559 +0xad
github.com/cosmos/relayer/cmd.(*Config).ValidatePathEnd(0xc000d710c0, 0xc000da4e70, 0x152b320, 0xc000db9ba8)
        /home/vitwit/go/src/github.com/cosmos/relayer/cmd/config.go:478 +0x85
github.com/cosmos/relayer/cmd.(*Config).ValidatePath(0xc000d710c0, 0xc000d9ee60, 0xc000c5fcf8, 0x12)
        /home/vitwit/go/src/github.com/cosmos/relayer/cmd/config.go:456 +0x47
github.com/cosmos/relayer/cmd.cfgFilesAdd(0x7ffc166859e6, 0xc, 0xc000c5fd50, 0x0, 0x0)
        /home/vitwit/go/src/github.com/cosmos/relayer/cmd/config.go:267 +0x377
github.com/cosmos/relayer/cmd.configAddDirCmd.func1(0xc0001c9600, 0xc000db2bc0, 0x1, 0x1, 0x0, 0x0)
        /home/vitwit/go/src/github.com/cosmos/relayer/cmd/config.go:185 +0x45
github.com/spf13/cobra.(*Command).execute(0xc0001c9600, 0xc000db2ba0, 0x1, 0x1, 0xc0001c9600, 0xc000db2ba0)
        /home/vitwit/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:850 +0x47c
github.com/spf13/cobra.(*Command).ExecuteC(0x22c9c80, 0x0, 0x0, 0x0)
        /home/vitwit/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:958 +0x375
github.com/spf13/cobra.(*Command).Execute(...)
        /home/vitwit/go/pkg/mod/github.com/spf13/cobra@v1.1.1/command.go:895
github.com/cosmos/relayer/cmd.Execute()
        /home/vitwit/go/src/github.com/cosmos/relayer/cmd/root.go:109 +0x4d
main.main()
        /home/vitwit/go/src/github.com/cosmos/relayer/main.go:21 +0x25

colin-axner

tested

fedekunze

Thanks for the fix. I'd suggest to return the errors when encountered

fedekunze · 2021-02-04T12:46:26Z

cmd/config.go

+			fmt.Printf("%s: %s\n", pth, err.Error())
+			continue


this should return the error

will update

fedekunze · 2021-02-04T12:46:42Z

cmd/config.go

+			fmt.Printf("failed to read file %s, skipping...\n", pth)
+			continue


fedekunze · 2021-02-04T12:46:58Z

cmd/config.go

+			fmt.Printf("failed to unmarshal file %s, skipping...\n", pth)
+			continue


fedekunze · 2021-02-04T12:47:04Z

cmd/config.go

+			fmt.Printf("%s: failed to validate path: %s\n", pth, err.Error())
+			continue


fedekunze · 2021-02-04T12:47:12Z

cmd/config.go

 			fmt.Printf("%s: %s\n", pth, err.Error())
 			continue


applied suggestions

WIP: Fix config add-dir regression

c84e1e3

colin-axner reviewed Feb 4, 2021

View reviewed changes

colin-axner changed the title ~~WIP: Fix config add-dir regression~~ Fix config add-dir regression Feb 4, 2021

colin-axner marked this pull request as ready for review February 4, 2021 10:58

colin-axner requested review from AdityaSripal and fedekunze as code owners February 4, 2021 10:58

colin-axner self-assigned this Feb 4, 2021

colin-axner marked this pull request as draft February 4, 2021 11:18

colin-axner added 2 commits February 4, 2021 13:14

split add-dir into add-chains and add-paths

33d674c

nicer error messages

ece2a00

colin-axner marked this pull request as ready for review February 4, 2021 12:44

colin-axner approved these changes Feb 4, 2021

View reviewed changes

fix lint

074e1b8

fedekunze previously requested changes Feb 4, 2021

View reviewed changes

apply @fedekunze suggestions

a53a0ab

colin-axner requested a review from fedekunze February 4, 2021 12:59

remove newline from error

c9d0dce

colin-axner merged commit d213300 into master Feb 4, 2021

colin-axner deleted the akhil/401-config-regression branch February 4, 2021 13:15

michaelfig mentioned this pull request Feb 4, 2021

fix: separate nchainz config/chains and config/paths #408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix config add-dir regression #406

Fix config add-dir regression #406

akhilkumarpilli commented Feb 4, 2021 •

edited by colin-axner

Loading

akhilkumarpilli commented Feb 4, 2021

colin-axner commented Feb 4, 2021 •

edited

Loading

colin-axner commented Feb 4, 2021

colin-axner Feb 4, 2021

akhilkumarpilli Feb 4, 2021

akhilkumarpilli commented Feb 4, 2021

colin-axner left a comment

colin-axner commented Feb 4, 2021

akhilkumarpilli commented Feb 4, 2021

colin-axner left a comment

fedekunze left a comment

fedekunze Feb 4, 2021

colin-axner Feb 4, 2021

fedekunze Feb 4, 2021

fedekunze Feb 4, 2021

fedekunze Feb 4, 2021

fedekunze Feb 4, 2021

		fmt.Printf("failed to read file %s, skipping...\n", pth)
		continue

		fmt.Printf("failed to unmarshal file %s, skipping...\n", pth)
		continue

		fmt.Printf("%s: failed to validate path: %s\n", pth, err.Error())
		continue

Fix config add-dir regression #406

Fix config add-dir regression #406

Conversation

akhilkumarpilli commented Feb 4, 2021 • edited by colin-axner Loading

akhilkumarpilli commented Feb 4, 2021

colin-axner commented Feb 4, 2021 • edited Loading

colin-axner commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhilkumarpilli commented Feb 4, 2021

colin-axner left a comment

Choose a reason for hiding this comment

colin-axner commented Feb 4, 2021

akhilkumarpilli commented Feb 4, 2021

colin-axner left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhilkumarpilli commented Feb 4, 2021 •

edited by colin-axner

Loading

colin-axner commented Feb 4, 2021 •

edited

Loading