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: pass prepared transaction to the SendConfirmation screen with navigate #102

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sviderock
Copy link

Description

This PR implements passing prepared transaction from SendEnterAmount to SendConfirmation with navigate to prevent unnecessary call to prepare transaction again. The call still can be called when navigating to SendConfirmation with Screen.SendConfirmationFromExternal.

Test plan

It doesn't load anything and doesn't show loading spinners when navigating to the SendConfirmation

Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2025-02-25.at.17.53.45.mp4

Related issues

Relates to RET-1300

Backwards compatibility

Yes

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

  • Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

}

export function usePrepareSendTransactions(
existingPrepareTranscation?: UsePrepareSendTransactions

Choose a reason for hiding this comment

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

i'm imagining how we'd document how to use this variable and it's pretty difficult to explain, and given the niche use case i think it's better not to expose this to builders? wdyt?

Copy link
Author

@sviderock sviderock Feb 26, 2025

Choose a reason for hiding this comment

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

There's no way to call useAsyncCallback optionally as it is a hook so it has to be defined in a declarative manner which means that it will be fired in any case. Unless we prevent it in the actual callback itself, like in this implementation. Or do you mean renaming the variable to just a boolean flag like disabledOnInit?

The other approach could be to wrap the whole SendConfirmation return into a HOC wrapper like <LoadPrepareTransaction> but IMHO this would be more cumbersome and complex.

Or is there any particular approach you were thinking about?

Choose a reason for hiding this comment

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

oh actually this is my bad, i misunderstood this as the exported prepareTransactions that we expose to builders. ignore this comment

const {
prepareTransactionsResult,
refreshPreparedTransactions,
clearPreparedTransactions,
prepareTransactionError,
prepareTransactionLoading,
} = usePrepareSendTransactions()
} = prepareTransactions

Choose a reason for hiding this comment

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

do we need to pass everything to the next screen? we don't allow proceeding unless the transaction is possible, so why not pass only prepareTransactionsResult? we may also want to serialise the prepared transaction (i vaguely remember that is recommended with this navigation lib)

@@ -29,10 +30,18 @@ type NestedNavigatorParams<ParamList> = {
: { screen: K; params: ParamList[K] }
}[keyof ParamList]

interface SendConfirmationFromExternalParams {

Choose a reason for hiding this comment

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

this could be simplified to Omit<SendConfirmationParams, 'preparedTransactions'>?

Copy link
Author

Choose a reason for hiding this comment

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

I did it this way so it would be possible to pass it directly into the hook without checking "prepareTransactions" in props.route.params as otherwise it is not available:
Screenshot 2025-02-26 at 14 29 33

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.

2 participants