-
Notifications
You must be signed in to change notification settings - Fork 103
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: improve delegation list header #8155
Conversation
…a2.0' of github.com:iotaledger/firefly into feat/improve-delegation-list-header
…a2.0' of github.com:iotaledger/firefly into feat/improve-delegation-list-header
…a2.0' of github.com:iotaledger/firefly into feat/improve-delegation-list-header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code looks great, just added some minor coding convention comments 🙏🏼
$: delegationOutputs = | ||
$selectedWallet?.walletUnspentOutputs?.filter((output) => output?.output?.type === OutputType.Delegation) || [] | ||
$: delegationOutputs?.length > 0 && mappedDelegationData(delegationOutputs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mappedDelegationData
looks like its a variable, this should be renamed to a getter or a setter or something
delegationOutputs?.map(async (output) => { | ||
const delegationOutput = output.output as DelegationOutput | ||
// Until the first epoch in which it was delegated ends, no rewards are obtained | ||
const epochsDelegating = currentEpoch - delegationOutput.startEpoch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldnt it be currentEpoch - delegationOutput.startEpoch - 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought like this, but when a delegation output is created, the startEpoch is currentEpoch + 1
In the tests I did, the currentEpoch
was 3 and the startEpoch
of the newly created delegation output was 4
return { | ||
[Header.Name]: `Delegation ${index + 1}`, | ||
[Header.DelegationId]: delegationOutput.delegationId, | ||
[Header.DelegatedFunds]: Number(delegationOutput.delegatedAmount), | ||
[Header.Rewards]: await getOutputRewards(output.outputId), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getOutputRewards
should not live in this component, i would move that logic out so its reusable somewhere else, these types of logics dont make sense in a svelte component
return Number(rewards) | ||
} | ||
|
||
async function getCurrentEpoch(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be moved somewhere else outside of this component too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🏇
Closes #8148
Closes #8106
Summary
...
Changelog
Testing
Platforms
Instructions
...
Checklist