Steps of stake #105

kkosiorowska · 2023-12-29T14:45:27Z

Closes #97

This PR implements the required steps to stake BTC. Currently, the SDK isn't yet available so let's mock these functions. SDK functions will be called in specially prepared hooks: useSignMessage and useDepositBTCTransaction. We will probably need one more hook responsible for sending the transaction and tracking its status. But let's do it when the SDK is ready. The scope of this PR ro prepare the right modals and place to connect to the SDK.

Minor note
The possibility to sign message has been added for testing. Probably this part will be removed and supported by the SDK. To properly go through the modals, you need to add the manifest file to the ledger live application once again.

Screen.Recording.2024-01-04.at.10.55.45.mov

ioay · 2024-01-04T15:26:28Z

dapp/src/components/shared/Spinner/index.tsx

+  SpinnerProps as ChakraSpinnerProps,
+} from "@chakra-ui/react"
+
+export default function Spinner({ ...spinnerProps }: ChakraSpinnerProps) {


Do we want the Spinner component? It seems to do nothing more than a chakra spinner, so in my opinion, we can use ChakraSpinner and let's create modalSpinnerSpeed const to keep the speed of the spinner for all modals.

In the future, we would like to create a library of components. For this reason, I have saved the default value of speed here. This allows us to easily determine the default speed of our spinner. Styleguide is still not fully ready therefore I would leave it as it is at this moment. I'm waiting for Sorin's opinion if something changes I will remove this component.

Sorin confirmed that each Spinner should have the same speed.

In a case when we have a plan to create a library of components(what chakra-ui is in itself) let's deal with it when there will be a requirement. Currently, such a wrapper adds an unnecessary layer of logic. In my opinion, it is more convenient to use the built-in chakra components and not wonder if we have any of our own, especially since we do not completely change the logic. We can add just a chakra-ui style variant to keep styling and to keep the spinner speed value as I mentioned above we should create just a modalSpinnerSpeed constant.

@r-czajkowski what do you think?

In this case, every time we use the spinner we have to remember to set the correct speed. I think that setting a constant speed in a shared component simplifies working with it. But I'm open to suggestions and understand your point of view.

In my opinion, extracting to const is more complicated than creating a wrapper. Where should we keep this const? const means it is something shared(at least for me) but in this case is strictly related to the spinner and currently, any component and our logic don't care about the spinner speed. Looks like it should be hidden under the Spiner component implementation.

Another option is creating a semantic token with this value and using it directly as we did for header height see semanticTokens.ts file.

import { Spinner } from "@chakra-ui/react" function MyComponent () { return ( <> ... <Spinner speed="<semantic-token-here>" /> ... </> ) }

We can also try to override the spinner style and force the animation speed but probably we will have to use !important which will be probably a huge pain in future development.

I see a big advantage in using a wrapper - e.g let's say we want to add an icon inside a spinner - we update only in one place and all spinners look the same.

So to sum up I'm ok with this wrapper but also understand Piotr and using const is also OK. I think there is no one good approach. But I would choose wrapper.

Summary

Let's stay with the wrapper at this moment. To avoid adding a constant speed for each spinner. However, depending on the feature changes, we can update this.

ioay · 2024-01-04T15:33:25Z

dapp/src/hooks/useSignMessage.ts

+  const { ethAccount } = useWalletContext()
+  const { signMessage, signature } = useSignMessageLedgerLive()
+
+  useEffect(() => {


Generally, I recommend to use a named function instead of using the anonymous arrow function in useEffect we can in an easy way without effort write self-documenting code, eg:

useEffect(function signMessageSucces(){ ... })

Thanks to this, if new useEffect hooks appear, we will know exactly what they are responsible for.

Our global eslint rules don't allow this. This logic will be updated anyway to use functions from the SDK. So let's just leave it as it is in this PR but think about the change later.

Can we add a prefer-arrow-callback rule for eslint? Naming effect functions in this way will make code management much easier in the future.

@r-czajkowski ?

It's a common pattern to use arrow functions in hooks and to be honest, I like it and I have never seen named function approach before. Usually useEffect callbacks aren't too complicated(and probably shouldn't be) and are easy to read. It is also a common pattern to create a function inside effect with a self-explanatory name eg:

useEffect(() => { const fetchSomething = async () => { ... } fetchSomething() })

Or we can hoist the complicated functions outside the component eg. to shared utils with a good name and use it in the hook.

So I'm arrow function team 😆 but ofc we can update the rule if we all agree on that.

I would also prefer to stay with what is there. As Rafał mentioned we can use the above pattern. So let's not exclude this rule.

ioay · 2024-01-04T15:34:11Z

dapp/src/theme/Spinner.ts

+  borderLeftColor: "gold.400",
+})
+
+const Spinner = defineStyleConfig({ baseStyle })


Suggested change

const Spinner = defineStyleConfig({ baseStyle })

const SpinnerStyle = defineStyleConfig({ baseStyle })

ioay · 2024-01-04T15:34:39Z

dapp/src/theme/Spinner.ts

+
+const Spinner = defineStyleConfig({ baseStyle })
+
+export default Spinner


Suggested change

export default Spinner

export default SpinnerStyle

Let's avoid the same export name in other files ([src/components/shared/Spinner/index.tsx] exports also Spinner)

Yes, you're right. Actually, a more precise name is spinnerTheme. Also, I think we should not use the default export in this case. This is a residue of our eslint rule which has been disabled. Let me know what you guys think.

59e9bf1

cc @r-czajkowski

I created a separate task for this to not lose the topic. #139 Depending on what we decide I will update the task and add it to our task list to correct these imports.

Let's use the same pattern as chakra so:

export const <component>Theme = {...}

or

const <component>Theme = {...} export default <component>Theme

Summary

Let's organize the imports by chakra pattern. It will be done in #139

ioay · 2024-01-04T15:41:27Z

dapp/src/components/Modals/Staking/index.tsx


-function StakingSteps() {
-  const { activeStep, goNext } = useModalFlowContext()
+function Staking() {


Let's keep with StakingStep or ActiveStakingStep, this name is more precise - shows what it returns.

ioay · 2024-01-04T15:48:11Z

dapp/src/components/shared/Alert/index.tsx

-    <ChakraAlert status={status} {...alertProps}>
-      {withIcon && <AlertIcon as={status ? ICONS[status] : undefined} />}
+    <ChakraAlert status={status} {...paddingRight} {...alertProps}>
+      {withAlertIcon && <AlertIcon as={status ? ICONS[status] : undefined} />}


Suggested change

{withAlertIcon && <AlertIcon as={status ? ICONS[status] : undefined} />}

{withAlertIcon && status && <AlertIcon as={ICONS[status]} />}

I wonder if it is definitely what we want. 🤔 But that's fine we can change it and possibly update it when needed.

08cbfe1

Previously, one of eslint's rules was to use default imports. However, we excluded this rule. Let's make the imports for theme components not default to make it easier to import the right component or correct theme. Also set a more precise name for the component theme to avoid mess.

Closes: #139 Changed default export theme to export const and updated naming convention themes consistent with the chakra-ui to represent what exactly export represents. Originally posted in #105 (comment)

Initial implementation of stake steps

e8faf95

kkosiorowska self-assigned this Dec 29, 2023

kkosiorowska added 5 commits January 3, 2024 13:08

Prepare custom action hooks for staking steps

fe8ac88

Create a custom theme for the spinner

755afbe

Add an action icon for the alert

a43d242

Merge branch 'main' of github.com:thesis/acre into steps-for-stake

88ba3d1

Simplify component logic for staking steps

d7312d2

kkosiorowska marked this pull request as ready for review January 4, 2024 10:09

kkosiorowska requested review from ioay and r-czajkowski January 4, 2024 10:09

ioay reviewed Jan 4, 2024

View reviewed changes

kkosiorowska force-pushed the steps-for-stake branch from 9bc532b to 59e9bf1 Compare January 8, 2024 10:18

kkosiorowska mentioned this pull request Jan 8, 2024

Imports update for component themes #139

Closed

kkosiorowska added 2 commits January 8, 2024 13:35

Use clearer name for staking content

b1683a1

Simplify the shared alert component

08cbfe1

kkosiorowska requested a review from ioay January 8, 2024 13:07

Merge branch 'main' of github.com:thesis/acre into steps-for-stake

cf0987c

ioay approved these changes Jan 10, 2024

View reviewed changes

kkosiorowska merged commit 3324435 into main Jan 10, 2024
11 checks passed

kkosiorowska deleted the steps-for-stake branch January 10, 2024 09:16

ioay added a commit that referenced this pull request Jan 10, 2024

Added new eslint rule related with the last #105 changes

25884ef

ioay mentioned this pull request Jan 17, 2024

Imports update for component themes #181

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Steps of stake #105

Steps of stake #105

kkosiorowska commented Dec 29, 2023 •

edited

Loading

ioay Jan 4, 2024 •

edited

Loading

kkosiorowska Jan 8, 2024

kkosiorowska Jan 8, 2024

ioay Jan 8, 2024

kkosiorowska Jan 10, 2024

r-czajkowski Jan 12, 2024

kkosiorowska Jan 12, 2024

ioay Jan 4, 2024

kkosiorowska Jan 8, 2024

ioay Jan 8, 2024

r-czajkowski Jan 12, 2024

kkosiorowska Jan 12, 2024 •

edited

Loading

ioay Jan 4, 2024

kkosiorowska Jan 8, 2024

ioay Jan 4, 2024

kkosiorowska Jan 8, 2024

kkosiorowska Jan 10, 2024

r-czajkowski Jan 12, 2024

kkosiorowska Jan 12, 2024

ioay Jan 4, 2024

kkosiorowska Jan 8, 2024

ioay Jan 4, 2024

kkosiorowska Jan 8, 2024

	const Spinner = defineStyleConfig({ baseStyle })
	const SpinnerStyle = defineStyleConfig({ baseStyle })


		const Spinner = defineStyleConfig({ baseStyle })

		export default Spinner

	{withAlertIcon && <AlertIcon as={status ? ICONS[status] : undefined} />}
	{withAlertIcon && status && <AlertIcon as={ICONS[status]} />}

Steps of stake #105

Steps of stake #105

Conversation

kkosiorowska commented Dec 29, 2023 • edited Loading

ioay Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska commented Dec 29, 2023 •

edited

Loading

ioay Jan 4, 2024 •

edited

Loading

kkosiorowska Jan 12, 2024 •

edited

Loading