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: add Increase Stake Modal #13

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

feat: add Increase Stake Modal #13

wants to merge 16 commits into from

Conversation

darklight81
Copy link
Contributor

@darklight81 darklight81 commented Jan 6, 2025

task link

Todo:
[x] Check if the functionality is ok
[x] decide where to put the button for opening the modal

Copy link

github-actions bot commented Jan 6, 2025

utils/useIncreaseStakeModal.ts Outdated Show resolved Hide resolved
pages/index.vue Outdated Show resolved Hide resolved
components/IncreaseStakeModal.vue Outdated Show resolved Hide resolved
components/IncreaseStakeModal.vue Outdated Show resolved Hide resolved
components/IncreaseStakeModal.vue Show resolved Hide resolved
Comment on lines 24 to 45
<div class="increase-stake-modal__submit">
<BaseTooltip
style="width: 100%"
v-if="isButtonDisabled"
:tooltip-text="tooltipText">
<template #trigger>
<button
class="increase-stake-modal__submit-button"
:disabled="isButtonDisabled"
@click="increaseStakeAction">
Confirm Increase
</button>
</template>
</BaseTooltip>
<button
v-else
class="increase-stake-modal__submit-button"
:disabled="isButtonDisabled"
@click="increaseStakeAction">
Confirm Increase
</button>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make this button a primary button (with teal color)
image

Copy link
Contributor

Choose a reason for hiding this comment

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

still relevant

components/BaseModal.vue Outdated Show resolved Hide resolved
components/IncreaseStakeModal.vue Outdated Show resolved Hide resolved
components/IncreaseStakeModal.vue Outdated Show resolved Hide resolved
components/IncreaseStakeModal.vue Outdated Show resolved Hide resolved
components/IncreaseStakeModal.vue Show resolved Hide resolved
utils/useIncreaseStakeModal.ts Outdated Show resolved Hide resolved
const currentLockUpEpochs = ref<number>(0)
const formattedStakeAmount = ref<string>()
export default function useIncreaseStakeModal() {
const openModal = (stakeIdVal: bigint, lockUpEpochs: number, stakeAmount: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if lockUpEpochs should be always int, imho we should call Math.floor in this function instead of places where we call this function (e.g. openModal(stake.id, Math.floor(stake.epochsRemaining), stake.amount) in TableOwnedPositions.vue

Copy link
Contributor

Choose a reason for hiding this comment

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

still relevant

formattedStakeAmount.value = stakeAmount
}

const calculateAvailableEpochs = (): number[] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: i'm not sure if it makes sense to display 0 epochs on the slider as option

image

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you think?

Increase Duration (in epochs)
</span>
</div>
<VueSlider
Copy link
Contributor

Choose a reason for hiding this comment

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

question: did you check if it's possible to make the slider values somehow non-linear in the switch between 5 years and 10 years, so there is actually bigger distance on the slider between the 5 years and 10 years prolong option?

components/IncreaseStakeModal.vue Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we should show date and not epochs as primary info for user, so e.g. on slider show new lockup date? feel free to check with Ork what makes most sense, but one of the feedback i received was to not use epochs as primary information for user and rather show them dates/durations, and if show epochs at all, then as a secondary information

Copy link
Contributor

Choose a reason for hiding this comment

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

bugs related to handling of transaction:

  1. button is not in disabled state when transaction is pending
  2. UI does not reflect updated positions, it should trigger refetch after the tx is completed
  3. we should show some success message to user

For handling the TX and check for confirmation, we can probably copy our sendTransaction method from FE repo that should also take care of the contract wallets compatibility?

Copy link
Contributor

Choose a reason for hiding this comment

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

after i've prolonged the duration of the stake, i got shown this message that I get the voting power in the next epoch... is that a correct behavior? if yes, then owner lose the voting power for the duration between increase and next epoch, right @ashhanai ? if yes, we should maybe put some note or something to inform the user that they will temporarily lose their voting power on that position?

image

Copy link
Member

Choose a reason for hiding this comment

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

They will not lose it as the voting power decrease is also happening in the next epoch. The current epoch is always immutable, you cannot increase nor decrease its voting power.

Comment on lines +230 to +231
console.log('user stakes', userStakes)
console.log('user stakes next', userNextEpochStakes)
Copy link
Contributor

Choose a reason for hiding this comment

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

lets remove?

Comment on lines +52 to +60
// console.log('Starting to send a transaction with following parameters:')
// console.log(transaction)
// console.log(`Additional UI parameter passed to sendTransaction: step=${step?.text}, safeAddress=${safeAddress}`)

const connectedChainId = getAccount(wagmiAdapter.wagmiConfig).chainId
console.log(`connectedChainId=${connectedChainId}; transaction.chainId=${transaction.chainId}`)

if (connectedChainId !== transaction.chainId) {
// console.log(`Switching chain from ${connectedChainId} to ${transaction.chainId}.`)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's uncomment these imho, they are nice for debugging

const currentLockUpEpochs = ref<number>(0)
const formattedStakeAmount = ref<string>()
export default function useIncreaseStakeModal() {
const openModal = (stakeIdVal: bigint, lockUpEpochs: number, stakeAmount: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

still relevant

Comment on lines +173 to +188
// TODO LATER: also allow for increasing amount, now only works to increase time
const increaseStakeAction = async () => {
if (!stakeId.value || !address.value || !additionalLockUpEpochs.value) return console.error('Missing required values')
isPending.value = true
await sendTransaction({
abi: VE_PWN_TOKEN_ABI,
address: VE_PWN_TOKEN[getChainIdTypesafe()],
functionName: 'increaseStake',
chainId: getChainIdTypesafe(),
args: [stakeId.value, address.value, 0n, BigInt(additionalLockUpEpochs.value)]
})
invalidateUserStakesQuery()
isPending.value = false
isOpen.value = false
//todo: refetch data
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be inside of useMutation, then we would not need to have separate isPending ref variable... also currently we do not handle correctly when user e.g. rejects the transaction - the button stays disabled with "Increasing..." text, which should also be resolved automatically when using useMutation


watch(isOpen, (newVal) => {
if (newVal) {
additionalLockUpEpochs.value = calculatedEpochs.value[0] || 0
Copy link
Contributor

Choose a reason for hiding this comment

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

should the additionalLockUpEpochs be computed?

invalidateUserStakesQuery()
isPending.value = false
isOpen.value = false
//todo: refetch data
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we can remove this comment now?

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.

3 participants