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

feat: migrate stacks generate txs, closes LEA-1732 #627

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

fbwoolf
Copy link
Contributor

@fbwoolf fbwoolf commented Nov 17, 2024

This PR migrates code from the extension to generate the unsigned stacks tx for the STX send flow.

Approach here now uses a separate Provider for each chain/protocol form so that we can handle loading data in the provider. I also did a version using our loader pattern, which seemed ok too, but felt using this approach might be more straightforward with using React context for the forms. We can add a loading state later, etc.

I also removed any helper functions I wasn't directly using with generating the unsigned tx. I realized I had ported over some unnec functions for this work. I got rid of all the stacks connect types.

I did add two new money formatters that can eventually help to replace stacksValue, thought I didn't end up needing them anywhere for now.

Copy link

linear bot commented Nov 17, 2024

Copy link

codecov bot commented Nov 17, 2024

Codecov Report

Attention: Patch coverage is 74.79675% with 31 lines in your changes missing coverage. Please review.

Project coverage is 24.75%. Comparing base (8d0b3dd) to head (741dba7).

Files with missing lines Patch % Lines
.../src/transactions/generate-unsigned-transaction.ts 73.33% 12 Missing ⚠️
packages/utils/src/money/format-money.ts 16.66% 10 Missing ⚠️
...kages/stacks/src/transactions/transaction.types.ts 54.54% 5 Missing ⚠️
packages/stacks/src/transactions/index.ts 0.00% 3 Missing ⚠️
packages/stacks/src/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #627      +/-   ##
==========================================
+ Coverage   23.76%   24.75%   +0.99%     
==========================================
  Files         165      169       +4     
  Lines        6194     6317     +123     
  Branches      337      361      +24     
==========================================
+ Hits         1472     1564      +92     
- Misses       4722     4753      +31     
Files with missing lines Coverage Δ
packages/stacks/src/stacks.utils.ts 97.53% <100.00%> (+2.18%) ⬆️
...es/stacks/src/transactions/post-condition.utils.ts 100.00% <100.00%> (ø)
packages/utils/src/money/calculate-money.ts 79.06% <ø> (ø)
packages/stacks/src/index.ts 0.00% <0.00%> (ø)
packages/stacks/src/transactions/index.ts 0.00% <0.00%> (ø)
...kages/stacks/src/transactions/transaction.types.ts 54.54% <54.54%> (ø)
packages/utils/src/money/format-money.ts 53.73% <16.66%> (-8.09%) ⬇️
.../src/transactions/generate-unsigned-transaction.ts 73.33% <73.33%> (ø)
Components Coverage Δ
bitcoin 62.04% <ø> (ø)
query 12.65% <ø> (ø)
utils 47.17% <16.66%> (-0.76%) ⬇️
crypto 68.21% <ø> (ø)
stacks 69.72% <81.08%> (+11.78%) ⬆️
---- 🚨 Try these New Features:

Copy link
Contributor

@pete-watters pete-watters left a comment

Choose a reason for hiding this comment

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

Nice work @fbwoolf 👍

There's some console logs + commented code to remove.

For the stacks package work, maybe we could add some of the functions into smaller independent files and generate some unit tests for them also?

Copy link
Collaborator

@edgarkhanzadian edgarkhanzadian left a comment

Choose a reason for hiding this comment

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

Great work!🚀

Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Great work @fbwoolf

Lots to discuss here though. I think there's some code, and general patterns here, we might want to reconsider bringing over if possible

packages/stacks/src/stacks.utils.ts Outdated Show resolved Hide resolved
packages/stacks/src/stacks.utils.ts Outdated Show resolved Hide resolved
packages/stacks/src/stacks.utils.ts Outdated Show resolved Hide resolved
packages/stacks/src/transactions/stacks-connect.types.ts Outdated Show resolved Hide resolved
@kyranjamie
Copy link
Collaborator

I used an approach where for each form we have a SendForm.StacksSetter (or appropriate chain/prototool) to set nec form data. We do this in the extension with NonceSetter. I think we can use this later to load/set fee values, etc. Open to other ideas here too.

I would investigate options where we do not even render the form components until we have this data loaded from the external source. Either the loader pattern, or some kind of route loader if Expo has those.

@fbwoolf
Copy link
Contributor Author

fbwoolf commented Nov 18, 2024

Great work @fbwoolf

Lots to discuss here though. I think there's some code, and general patterns here, we might want to reconsider bringing over if possible

Yep, agree, I was debating abt just doing a draft PR but wasn't sure how quickly we wanted to push the send form forward. I'll change this to draft and tackle some of the refactoring you pointed out. I mostly wanted to make sure I got feedback on effort here so far. 👍

@fbwoolf fbwoolf marked this pull request as draft November 18, 2024 13:54
@fbwoolf fbwoolf force-pushed the feat/migrate-stacks-generate-txs branch 2 times, most recently from f99a878 to 1f9ea83 Compare November 19, 2024 20:16
@fbwoolf fbwoolf force-pushed the feat/migrate-stacks-generate-txs branch 3 times, most recently from 81d205b to d869a60 Compare November 20, 2024 01:06
@fbwoolf fbwoolf force-pushed the feat/migrate-stacks-generate-txs branch 2 times, most recently from db07cf2 to 93a6ffc Compare November 20, 2024 14:54
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Nov 20, 2024

I am going to open this for review again. Open to feedback on using the separate providers here vs our loader pattern. I'm happy to switch if people prefer the loader pattern. Expo router doesn't offer loaders.

@fbwoolf fbwoolf marked this pull request as ready for review November 20, 2024 14:59
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Nov 20, 2024

Working a bit more here to split up the providers bc currently they are named incorrectly. They need to be protocol specific.

@fbwoolf fbwoolf force-pushed the feat/migrate-stacks-generate-txs branch from 93a6ffc to 3b5e1fb Compare November 20, 2024 18:06
@fbwoolf fbwoolf force-pushed the feat/migrate-stacks-generate-txs branch from 3b5e1fb to da70099 Compare November 20, 2024 18:43
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Nov 20, 2024

Looking at this closer, it might be that we want separate providers per protocol for flexibility, but use the loader pattern per chain so we don't duplicate efforts with loading chain data like fees and nonces. Might try adding that as a next step.

Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Curious to hear your thoughts on feedback I've left, but very impressed with this solution. Think you've struck a great balance between reusability of the form, and compatibility of all the elements with the context.

Comment on lines +15 to +20
export type ReplaceTypes<T, Replacements extends { [K in keyof T]?: any }> = Omit<
T,
keyof Replacements
> & {
[K in keyof Replacements]: Replacements[K];
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this do?

Copy link
Contributor Author

@fbwoolf fbwoolf Nov 20, 2024

Choose a reason for hiding this comment

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

I wanted to reuse part of the @stacks/transactions types but replace their use of IntegerType from @stacks/common with our Money type and narrow the other types rather than installing another lib to handle their IntegerType. I could completely create our own types copied from @stacks/transactions, but I looked up this utility to replace types. Now, we can pass our Money type all the way to the stacks pkg and then just convert it to a string there before using their typed function. Look at use in transaction.types.ts.

packages/stacks/src/stacks.utils.ts Outdated Show resolved Hide resolved
packages/stacks/src/transactions/post-condition.utils.ts Outdated Show resolved Hide resolved
const stxAddress = stxSigner?.address ?? '';
const { data: nextNonce } = useNextNonce(stxAddress);

if (!account || !stxSigner) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any way to handle these missing values differently, such that by the time we get to this hook, we're sure they're defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up creating a loader for the StacksSigner and passing the signer address and publicKey hex thru the route params. Hoping this is a good solution here?

defaultStacksFees.estimates[0]?.fee ?? defaultFeeFallbackAsMoney
);

if (!nextNonce) return null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we have a nonce loader component, or even a loader component specific to this send form?

I could see the Loader pattern living side by side use of context here.

<StxSendFormLoader fallback={<LoadingScreen />}>
  {nonceEtc => <SendFormProvider {...nonceEtc} />}
</StxSendFormLoader/>

Copy link
Contributor Author

@fbwoolf fbwoolf Nov 20, 2024

Choose a reason for hiding this comment

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

Yep, agree, was refactoring to this when you were reviewing. I was thinking maybe we only need loaders per chain, but maybe we do need per protocol. 🤔 Stacks would load the same fees and nonce for stx and sip10, so there would be some duplication there if do separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking should be separate, so separating. I think the loader pattern makes more sense to me rather than using the hook for loading, but open if others feel strongly either way.

@fbwoolf fbwoolf force-pushed the feat/migrate-stacks-generate-txs branch 4 times, most recently from a31a7cf to 1b127cd Compare November 21, 2024 18:51
@fbwoolf
Copy link
Contributor Author

fbwoolf commented Nov 21, 2024

@kyranjamie hoping to get your feedback on how I've addressed your questions. I am now handling all undefined values thru a form loader component, which I think is working well.

Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Fara

@fbwoolf fbwoolf force-pushed the feat/migrate-stacks-generate-txs branch from 1b127cd to 741dba7 Compare November 22, 2024 01:50
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.

5 participants