Staking changes

[apporc]

No description provided.

[cloudflare-workers-and-pages]

Deploying 2nicove with    Cloudflare Pages

Latest commit:	3f5a0b2
Status:	✅  Deploy successful!
Preview URL:	https://5b6bbafc.unicove2.pages.dev
Branch Preview URL:	https://apporc-staking.unicove2.pages.dev

View logs

[dafuga]

Suggested change

	return Asset.fromUnits(claimable, network.chain.systemToken!.symbol);
	return Asset.fromUnits(claimable, network.chain.systemToken?.symbol || '4,UNKNOWN);

[dafuga]

I would do something like this everywhere that you're using the ! operand.

[apporc]

Just checked that we are using this.chain.systemToken.symbol almost everywhere, without ! or ?. Your IDE doesn't report the error or you ignore it?

[dafuga]

Oh yeah, that was my bad, it was because I was using an old version of the wharfkit/common package 😅 I have that all cleaned up in my latest PR 👍

[dafuga]

I still think that this doesn't belong in a state file.

[apporc]

I could use another class name like WithdrawManager, instead of xxxState, lol. Yeah i agree a pure state should not hold more logics, but here i think what we need is not a pure state. I know how you've wrote other pages, like ram and send pages, read all about them before coding this, but i think this approach in staking is more clear and easy. Tell me which one you like, WithdrawManager or a pure state and dissecting it in another page file? Keep in mind that, there is consistently only 5 lines of code in every page file, it's fully abstraction between ui and logic :

	const { data } = $props();

	let stakeState: StakeState = $state(new StakeState(data.network));

	$effect(() => {
		stakeState.sync(data.network, context.account, context.wharf);
	});

[dafuga]

Renaming it would be a step in the right direction, but my other issue with your approach is that it's not consistent with what we've been doing on the other pages.

[dafuga]

How about you follow the state class format that we have in the other pages, but you can also have another class to manage the action handling? Maybe it can even take a state class instance as a param? The result would be similar to what you have here, but more consistent with the other pages.

[apporc]

I don't think state and state manager should be separated. They are just class's members and class's method properties. Why would we make them two classes?
If you think the withdrawManager is the right direction, why not using this approach in all other pages?

[dafuga]

The reason why you would want two separate classes IMO is because you'd want to decouple the page state from the handling of the transaction since a) we may want to reuse those transaction classes elsewhere and b) I think that having a state class for each page (which is only used to store the page state and nothing else) will make it easier for our future selves to figure out what is going on on any given page.

[aaroncox]

Just to chime in, the only reason we are using a class is because its required if we want to export those Svelte runes from a separate file. If that wasn't a requirement and we could just export them all as individual runes, we probably would and we wouldn't even have any classes like this (e.g. a "manager").

I'm glad we're having this conversation because we do need to settle on a design pattern for building these pages. I'm not sure what approach is best yet... but the it's kinda awesome that each of these sections is like it's own mini-app and we can experiment with them independently!

I'm going to merge this... but lets keep this conversation going.