Skip to content

Comments

Client refactor#1

Merged
eyedeekay merged 6 commits intogo-i2p:masterfrom
urgentquest:client-refactor
May 11, 2025
Merged

Client refactor#1
eyedeekay merged 6 commits intogo-i2p:masterfrom
urgentquest:client-refactor

Conversation

@urgentquest
Copy link

@urgentquest urgentquest commented Mar 9, 2025

  • Fix lots of TODOs by implementing client-side reply validator func validateReply().
  • While I am here: add IsOk() and GetResult() to replyParser.go. both are quite frequently used in the library
  • Handle edge cases with no username/password in Client.hello()
  • Replace fmt.ErrorF() with errors.New() where no formatting requested
  • go-staticcheck

@eyedeekay
Copy link

Yeah, simplify simplify simplify. I assume you ran all of the go tests with go test --tags nettest?

@urgentquest
Copy link
Author

If I recall correctly I did, but let me double check this. Just to be sure

- Replace fmt.ErrorF() with errors.New() where no
actual formatting requested
- Hanle edge cases with no username/password in `Client.hello()`
- Make go-staticcheck happier
@urgentquest
Copy link
Author

Well. There were some complications. Don't know how I missed that.
Still there is a single test failing

Initializing new ID: 2095128466
2025/05/09 00:43:36 > "HELLO VERSION MIN=3.0 MAX=3.1\n"
2025/05/09 00:43:36 < "HELLO REPLY RESULT=OK VERSION=3.1\n"
2025/05/09 00:43:36 > "NAMING LOOKUP NAME=!(@#)\n"
2025/05/09 00:43:36 < "NAMING REPLY RESULT=INVALID_KEY NAME=!(@#)\n"
--- FAIL: TestClientLookupInvalid (0.00s)
    naming_test.go:29: client.Lookup() should throw an ResultKeyNotFound error.
        Got:ReplyError: Result:INVALID_KEY - Reply:&{Topic:NAMING Type:REPLY From: To: Pairs:map[NAME:!(@#) RESULT:INVALID_KEY]}!=KEY_NOT_FOUND

Apparently i2pd I'm running says invalid key instead of result key not found which causes the failure. I could work around this by adding an extra condition. Or just leave is as it is.

@eyedeekay
Copy link

Thought about it, go ahead and add the extra condition for i2pd. I think we need to be compatible with both.

@urgentquest
Copy link
Author

Done.
All green

@eyedeekay
Copy link

Excellent thanks again for your help/contribution. Looks good, I'm ready to merge.

At some point we might be ahead to build a SAM API test harness which runs a fake SAMv3 server that replies with accurate errors. I have one started around here in the go-i2p/go-sam-server namespace but it's never been a big priority, as we'd have to develop it, make sure it behaves exactly as the Java AND I2P SAMv3 servers, and keep up with the SAMv3 spec, but it would let us run the whole test suite in CI and catch stuff like this. Maybe the calculus on that should change soon.

@eyedeekay eyedeekay merged commit 46d55e7 into go-i2p:master May 11, 2025
@urgentquest urgentquest deleted the client-refactor branch June 9, 2025 21:53
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.

2 participants