Fix: pass session key instead of full authority data in Ed25519SessionAuthority methods - #117
Conversation
tracy-codes
left a comment
There was a problem hiding this comment.
overall this LGTM. can you please add some regression tests just to ensure this never pops up again in the future?
| { | ||
| roleId: args.roleId, | ||
| authorityData: this.data, | ||
| authorityData: this.sessionKey.toBytes(), |
There was a problem hiding this comment.
can we add a regression test for this exact path?
please cover Ed25519 session authority + getCreateSubAccountInstructionContext(...) (or classic getCreateSubAccountInstructions(...)) and assert it no longer throws Invalid PublicKey byte length and successfully creates the sub-account PDA.
| actingRoleId: args.actingRoleId, | ||
| actions: args.actions.bytes(), | ||
| authorityData: this.data, | ||
| authorityData: this.sessionKey.toBytes(), |
There was a problem hiding this comment.
since this PR updates multiple Ed25519 session methods from this.data (80-byte struct) to this.sessionKey.toBytes() (32-byte key), can we add one manage-authority regression test as well (e.g. add-authority via session role) to lock this behavior in?
There was a problem hiding this comment.
Absolutely, I will add some tests soon!
| { | ||
| roleId: args.roleId, | ||
| authorityData: this.data, | ||
| authorityData: this.sessionKey.toBytes(), |
There was a problem hiding this comment.
Oh yes. Thanks for this! Although, these should be .id() or address() with the new methods. not .sessionKey.toBytes(). Nice catch
There was a problem hiding this comment.
@thewuhxyz - Let me know if I'm wrong, but I'm seeing this.id -> this.info.publicKey. this.address -> this.ed25519PublicKey, and this.signer -> this.sessionKey.toBytes(). Should we be using this.signer here?
There was a problem hiding this comment.
this.signer is just for instructions that sign, like subAccountSign and sign, this.id/address should be used for other instruction, mostly.
There was a problem hiding this comment.
Ok switched 'em all over to this.id. Let me know how it looks.
There was a problem hiding this comment.
@thewuhxyz - any other changes you'd like to see in this PR? Thanks for looking!
There was a problem hiding this comment.
Looks good to me. Just wondering the cause of the hiccup in CI
There was a problem hiding this comment.
I'm not sure, do you think it's related to my changes?
There was a problem hiding this comment.
I reverted the last commit to see if it would address the failing tests. Can add back if necessary, lmk
1c6295f to
d6bf6c2
Compare
|
@wirewallet-jeremy please rebase your fork against latest main here and push up, some changes to CI were made to support solana v3.x.x. that should trigger the action to run again and pass |
Will do @tracy-codes. Thanks for looking! |
… passes the full 80-byte authority data to `new PublicKey()`, which expects 32 bytes and crashes with 'Invalid PublicKey byte length. Length is 80, not 32 bytes.' Instead of passing the entire 80-byte serialized authority struct, just pass the 32 byte session key.
* session authority can create sub-account after session activation * session authority builds addAuthority instruction without byte-length error
d6bf6c2 to
07e6313
Compare
2282f11 to
5e15268
Compare
I hit this trying to use the SDK to create sub-accounts under a Swig smart wallet.
Stacktrace
This attempts to fix the issue where the SDK's
_Ed25519SessionAuthority.subAccountCreatepasses the full 80-byte authority data tonew PublicKey(), which expects 32 bytes and crashes with 'Invalid PublicKey byte length. Length is 80, not 32 bytes.'Instead of passing the entire 80-byte serialized authority struct, just pass the 32 byte session key.
Script which should reproduce the issue: