Open for some refactor?

[Edznux]

Hey,

I was looking at the project a bit and was wondering if you were open for some refactoring of the code-base (before I commit to much time on it if you don't want to).

These code changes would follow the generic Go guidance available here, or earlier, simpler here

At a first glance this could includes updates such as:

• return early
• Guard Clause
• unit tests (and other kind of tests, if deemed useful (e.g: go 1.18 fuzzing potentially))
• definition of constants variables

Some higher level refactoring could be further discussed at a later point (e.g: creating a shared go.mod to ensure cross compatibility between binaries, simpler code sharing, folder restructure to match a more "Go idiomatic" way)

I believe this would help maintenance on the long term and people on boarding the project on a shorter term.

Example PR: #285

[its-a-feature]

Hey! I love the enthusiasm! I'm definitely open for refactoring and making things easier for the community to help maintain/contribute.

I'll need to dive into the test piece more for sure. Is there a reason you removed error logging and references to the local logging package?

I think the current format returns early when errors are encountered just by the nature of the collapsed if var, err := doSomething(); err != nil structure, but in the more complex functions, like handleAgentMessageStagingRSA, I think it can be much easier to read in your refactored version.

[Edznux]

Cool, I'll make sure my draft is complete and will open it up for reviews then!

Is there a reason you removed error logging and references to the local logging package?
The reason here is to log the error with all its context (via the %w format) in the error and then leave it to the responsibility of the caller to properly log, with the correct level as well when erroring out.
This avoid multiple edge cases:

• printing with incorrect levels: the pem.Decode(publicKey) snippet seems like a good example here, you have a fallback to decode; so it's functioning, maybe not as good as it could, but the error should most likely be a info; or a warn level, not error?
• duplicating logs (print the error in the function then print it again in the caller (and all the way up to the chain))
  • This can also lead to intertwined logs via concurrency (if you print multiple times the log and a concurrent go-routine ran in between the two, the context might be more difficult to read)
• shorter, more consistent (and less error prone) error path: the content of the error is always the same. (And if we explicitly don't want to print to the user that content, we can at check)
• Depending on how its printed, it may help reducing noise (in tests but also during the runtime).

You can see it described here in the Uber golang style guide

I think the current format returns early when errors are encountered just by the nature of the collapsed if var, err := doSomething(); err != nil structure, but in the more complex functions, like handleAgentMessageStagingRSA, I think it can be much easier to read in your refactored version.

So, inline error checks definitely has it place!
In the draft PR, I had to split them in two because of the variable scope. Since you used an else statement, the scope of the variable was correct.
If we want to use "guarded close", we need to be able to use the outputs of the functions.
Here's a playground example: https://go.dev/play/p/RVt7WsLwC8r

We could instead declare the variable and assign them with = instead of := like this:

	var newKey string
	var err error
	if newKey, err = mythicCrypto.GenerateKeysForPayload("aes256_hmac"){
		return nil, fmt.Errorf("failed to generate new AES key for staging rsa: %v", err)
	}

You can see an example in the Uber style guide here

If the function, only returns something that doesn't need to be used later on (error, or _ most likely), the inline check works perfectly fine.

[its-a-feature]

Sorry it's taking me so long to get around to this! I kept meaning to merge them in, but then would get side tracked with other tasks. As I've been adding new features, I've been trying to keep the principals you described in mind though. I merged in two of your PRs so far

[Edznux]

No worries, thanks for merging them!
I'm not sure I'll have much time to spend on this anymore but I'll keep an eye :)