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

Factor out the implementations and re-add the existing constructor #1

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

Rob-Hague
Copy link

@Rob-Hague Rob-Hague commented Nov 12, 2023

This brings it closer to the shape described in sshnet#865 (comment). I figured it was easier to show than tell.

There are no functional changes.

I have further feedback after this. Would you prefer I address them in PRs like this one or leave as a review for you to address?

Thanks

P.S for the avoidance of doubt: this PR targets your PR branch so if you merge this it will show up automatically in sshnet#865

@zybexXL
Copy link
Owner

zybexXL commented Nov 13, 2023

Just add a code review on the original PR if needed.
The original constructor is not needed, why add it back?
Factoring out the CTR implementation, as I mentioned in another comment, is a bit weird unless the other modes also have their own class - but they would be empty classes.

@Rob-Hague
Copy link
Author

Just add a code review on the original PR if needed.

I figured I should do the work rather than asking it of you.

The original constructor is not needed, why add it back?

I'm cautious about breaking changes.

Factoring out the CTR implementation, as I mentioned in another comment, is a bit weird unless the other modes also have their own class - but they would be empty classes.

It is a different implementation than the other modes. It doesn't seem weird to me.

{
var keySize = key.Length * 8;

if (keySize is not (256 or 192 or 128))
Copy link
Owner

Choose a reason for hiding this comment

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

Why not leave this check in place?

Copy link
Author

Choose a reason for hiding this comment

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

The S.S.C.Aes object will perform the same check

@zybexXL
Copy link
Owner

zybexXL commented Nov 13, 2023

Rob, I like this refactoring as it makes the code cleaner, and we did discuss refactoring CTR into its own class. Please make the small edits and I'll merge it in.

@Rob-Hague
Copy link
Author

Oh dear, I shouldn't have merged develop into this branch. Will revert.

@zybexXL zybexXL merged commit d53c12c into zybexXL:feature_AES_CSP Nov 20, 2023
@Rob-Hague Rob-Hague deleted the feature_AES_CSP branch November 20, 2023 17:22
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