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.

Sign up for GitHub

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

Jump to bottom

Fix/sign transaction type #1097

Merged
merged 15 commits into from
Nov 13, 2024
Merged

Fix/sign transaction type #1097

merged 15 commits into from
Nov 13, 2024
+157 −34

Conversation

BlaineHeffron
Copy link
Contributor

Convert signTransaction and signAuthEntry types used in AssembledTransaction to new Sep43 spec.

Resolves #1054

Note that for now it adds a dependency for the "@creit.tech/stellar-wallets-kit" package for the new types. This should be changed to the Wallet SDK once it has been updated to export these types.

Currently for testing I just changed the basicNodeSigner to match the new argument and return types, which makes all the tests pass.

@socket-security Socket Security
Copy link

socket-security bot commented Nov 12, 2024 •
edited
Loading

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

chadoh
chadoh approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

A couple nits, but mostly this looks good. Thank you, @BlaineHeffron!

src/contract/assembled_transaction.ts Outdated Show resolved Hide resolved
src/contract/basic_node_signer.ts Outdated Show resolved Hide resolved
src/contract/types.ts Outdated Show resolved Hide resolved
src/contract/types.ts Outdated Show resolved Hide resolved
@BlaineHeffron
Copy link
Contributor Author

Fixed @chadoh's comments. Although it did bring up a point that I forgot to mention - the type in stellar-wallets-kit doesn't return an optional Error type as is shown in the Sep43 spec. We could either implement the type ourselves or we could just leave it as is and update it once its in the Wallet SDK. Thoughts?

@chadoh
Copy link
Contributor

chadoh commented Nov 12, 2024

Ahhh, what a bummer. This is a big breaking change; changing the interface again later is going to be another big breaking change. Best to only have one!

I think SEP-43 is the standard, and wallets have started implementing it. For example, Freighter returns the optional error. If the types a developer imports from their wallet library mismatch the types in the js sdk, that's a bug. We need to avoid it.

Let's patch the stellar-wallets-kit type to match SEP-43.

chadoh
chadoh reviewed Nov 12, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@Shaptic
Copy link
Contributor

Shaptic commented Nov 13, 2024

Can you rebase this onto master? It seems like there's a lot of irrelevant changes in the commit log. Also, would you say this is a blocker for v22 stable release? If so please prioritize it 🙏

@BlaineHeffron
Copy link
Contributor Author

Can you rebase this onto master? It seems like there's a lot of irrelevant changes in the commit log. Also, would you say this is a blocker for v22 stable release? If so please prioritize it 🙏

Yeah I'll fix it, I rebased onto master but something weird happened...

BlaineHeffron and others added 12 commits November 13, 2024 17:28
@BlaineHeffron
update signTransaction and signAuthEntries to Sep43 protocol
e13fad8
@BlaineHeffron
add documentation for new args
95c98fe
@BlaineHeffron
modify basic node signer to match new Sep43 spec
1299fee
@BlaineHeffron
add changelog, proper error type for basic node signer
4f8e6c1
@BlaineHeffron
formatting
0020689
@BlaineHeffron
preserve original submit option
3514a37
@BlaineHeffron @chadoh
Update src/contract/types.ts
fea40ef 
format

Co-authored-by: Chad Ostrowski <221614+chadoh@users.noreply.github.com>
@BlaineHeffron @chadoh
Update src/contract/types.ts
e1d42c8 
more clear wording

Co-authored-by: Chad Ostrowski <221614+chadoh@users.noreply.github.com>
@BlaineHeffron
formatting
cc9b7af
@BlaineHeffron
use imported type for basicNodeSigner
7021bae
@BlaineHeffron
remove stellar-wallets-kit dep and add optional error return field / …
115f1a3 
…error handling
@BlaineHeffron @chadoh
Update CHANGELOG.md
c91261e 
more description

Co-authored-by: Chad Ostrowski <221614+chadoh@users.noreply.github.com>
@BlaineHeffron
Copy link
Contributor Author

@Shaptic Should be fixed now!

Shaptic
Shaptic approved these changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Nice, super easy to read now! Looks great! And ty for the changelog 👏

@Shaptic Shaptic merged commit 40f2175 into stellar:master Nov 13, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chadoh chadoh chadoh approved these changes

@Shaptic Shaptic Shaptic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
DevX
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

signTransaction typing no longer matches Freighter / SEP-43
3 participants
@BlaineHeffron @chadoh @Shaptic