Closed arleypadua closed 3 months ago
I'm not sure having it injected all the way up at the top is the right way...
There isn't a need for flexibility during runtime to use one or the other, instead it's moreso a complete replacement of the provider. I'd think that having a singleton inside the MemeCrypto class that an API consumer can replace is all that's needed.
Also, HOME uses AES, so it might be worthwhile for a replacement for that. Same might apply for BD/SP's use of MD5.
I'm not sure having it injected all the way up at the top is the right way...
There isn't a need for flexibility during runtime to use one or the other, instead it's moreso a complete replacement of the provider. I'd think that having a singleton inside the MemeCrypto class that an API consumer can replace is all that's needed.
Also, HOME uses AES, so it might be worthwhile for a replacement for that. Same might apply for BD/SP's use of MD5.
@kwsch thanks for the feedback, indeed it seems like a static aspect of the MemeCrypto
as it is not intended to change in any way, unless once depending on the runtime
Moved it to a singleton + added the Md5 hasher to it
Revised HOME and BD/SP usages to make extraction of an interface more sane.
Revised HOME and BD/SP usages to make extraction of an interface more sane.
The tractor looks good and cleaner with methods for their specific cipher mechanism.
The only missing aspect for me if that I feel that leaving the properties of RuntimeCryptographyProvider.cs
with public setters may be a bit dangerous without applying constraints (see this this comment: https://github.com/kwsch/PKHeX/pull/4311#discussion_r1661283426)
I intended to allow changing the default implementation, but if that has been changed already to something else, do not allow changing it further.
This would protect the feature by making sure that if it is changed, it has to be changed only once, ideally at the start of the app.
The counter part reasoning is that it may be too much guessing for a niche, this irrelevant
Any thoughts?
I don't think it's any issue either way, just that it's less to look at as a public setter. There are numerous other instances where features can be replaced (like applying shape markings to entities), but there hasn't been a case where customizations clash.
For this case, it's usually only ever going to be replaced by the startup environment, and any sort of plugin wouldn't touch it. It's kinda like dependency injection of a service.
I don't think it's any issue either way, just that it's less to look at as a public setter. There are numerous other instances where features can be replaced (like applying shape markings to entities), but there hasn't been a case where customizations clash.
For this case, it's usually only ever going to be replaced by the startup environment, and any sort of plugin wouldn't touch it. It's kinda like dependency injection of a service.
Then I guess it is fine...
From the state of this MR I can tell that it fulfills the requirements of the app by allowing me to replace the implementation of the cryptography/hashing functions.. so from my side all good ✅
@kwsch a last question, what's usually the frequency of publishing a new nuget package version, so I can properly follow up on the other side?
I usually try to push a release once a month, and with the current pipeline setup I can release an intermediary NuGet if needed.
Overdue for a new release; I'll find time later today to push both.
I usually try to push a release once a month, and with the current pipeline setup I can release an intermediary NuGet if needed.
Overdue for a new release; I'll find time later today to push both.
that's awesome.. thanks
Description
Allows external consumers of
PKHeX.Core
to specify an implementation of the AES algorithmMotivation
I recently started to develop a web-based pokemon save editor based on
PKHeX.Core
, which is a really great library put out there to the community.This save editor runs on a web browser by using Blazor + Web Assembly.
The issue is that recently I received a feedback from @santacrab2 that I would encounter issues with US/UM save files, as they are encrypted
And indeed, putting it to test, I noticed that the following error is thrown when running
PKHeX.Core
through web assembly on a browser and exporting a US save file:It turns out that Microsoft, although allows to use the whole API surface of the .NET Framework in a WASM project, during the runtime, they disabled the native support to some APIs under the
System.Security.Cryptography
namespaceWith this change, one can specify
options
in which they want a save file to be loaded, and within it, one can specify an implementation of theAES
algorithmGoals
Example
An example of a different implementation of the aes provider can be found here: https://github.com/arleypadua/PKHeX.CLI/pull/44/files#diff-a7d741fed798d83c0e5ac2b80058c858acb3f0a463199a188102f3c124fb9579R5