BREAKING CHANGE(fix): deriveKeypair ignoring a manual algorithm being specified

[JST5000]

High Level Overview of Change

ripple-keypair's deriveKeypair function was ignoring when algorithm was passed in as an opt. Instead it incorrectly interpretted all seeds without an sEd start as using the secp256k1 algorithm.

Pre-fix, this shows up in xrpl.js in that Wallet.fromSeed('sAbc...', { algorithm: ECDSA.ed25519 }) will always use secp256k1 instead of ed25519 and Wallet.fromSeed('sEdXyz...', { algorithm: ECDSA.secp256k1 }) will always use ed25519 since that's what the "seed prefix" indicates to do.

Context of Change

This is a very breaking fix, and so should wait until 3.0 since it would cause all manually defined seeds to be interpretted via ed25519 instead of secp256k1. This PR would cause existing seeds to not work.

In order to get them to work again, the code would have to be updated to specify { algorithm: ECDSA.secp256k1 } when generating the Wallet using any of the generators (for example Wallet.fromSeed(sN2abs..., { algorithm: ECDSA.secp256k1 }).

Additionally, xrpl's Wallet.generate() was depending on this automatic seed evaluation behavior, and so has been updated to work with the fix.

Found while updating the secretNumbers PR #1799

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Test Plan

Updated CI passes

• I replaced individual seeds with a common seed with known public/private keys for both secp256k1 and ed25519 encodings to simplify adding test cases.
• For the rest of the tests that used their own seed, I simply added { algorithm: ECDSA.secp256k1 } to ensure the same account as before was used to sign / compare against expected values.

[ckniffen]

While this is new behavior this a major fix. I would be okay with putting this in 3.0 and releasing before the other 3.0 features and calling that 4.0

[mvadari]

This doesn't need to be a breaking change - you can just continue to do pre-existing behavior when the algorithm is not specified, like in XRPLF/xrpl-py#415

[mvadari]

While this is new behavior this a major fix. I would be okay with putting this in 3.0 and releasing before the other 3.0 features and calling that 4.0

It would actually be a 2.0 for ripple-keypairs and the other libraries wouldn't need to be bumped

[sublimator]

Just seeing this on mobile email - and likely missing some context - but it should be easy to determine algorithm via length of the decoded base58. Ed25519 has a 3 byte version iirc ?

…

On Wed, Jul 12, 2023, 7:33 AM Mayukha Vadari ***@***.***> wrote: While this is new behavior this a major fix. I would be okay with putting this in 3.0 and releasing before the other 3.0 features and calling that 4.0 It would actually be a 2.0 for ripple-keypairs and the other libraries wouldn't need to be bumped — Reply to this email directly, view it on GitHub <#2376 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEAHG4GULNL4WRZWSUG7VLXPXWGJANCNFSM6AAAAAA2GXFJCU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[mvadari]

Just seeing this on mobile email - and likely missing some context - but it should be easy to determine algorithm via length of the decoded base58. Ed25519 has a 3 byte version iirc ?

That is only true of the public/private key - seeds don't have types

[JST5000]

@mvadari In theory I think this change makes this non-breaking by restoring the default to using the "interpret based on seed" behavior, but I'm nervous about it - bf1be2f

I'd prefer to just fully make the changes in a breaking release since best case scenario if we release it in a non-breaking version, this change changes nothing for existing users. Worst case scenario breaks someone's ability to access their wallet, what do you think?

[mvadari]

@mvadari In theory I think this change makes this non-breaking by restoring the default to using the "interpret based on seed" behavior, but I'm nervous about it - bf1be2f

I'd prefer to just fully make the changes in a breaking release since best case scenario if we release it in a non-breaking version, this change changes nothing for existing users. Worst case scenario breaks someone's ability to access their wallet, what do you think?

This fix is now being added to 3.0, so it's fine to make it breaking.

[ckniffen]

@JST5000 This needs to have merge conflicts addressed. That should clean up some of the extra updates in this PR like typescript version.

[JST5000]

(Note: I will update the secretNumbers test TODO in a separate PR now that this is merged)