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

Add undelegate, redelegate and claim rewards #95

Merged
merged 11 commits into from
Apr 13, 2023

Conversation

abefernan
Copy link
Contributor

@abefernan abefernan commented Apr 12, 2023

Part of #52.
Closes #74.

@abefernan abefernan self-assigned this Apr 12, 2023
@vercel
Copy link

vercel bot commented Apr 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
cosmos-multisig-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 13, 2023 11:58am

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Good stuff!

<>
<li>
<label>Amount:</label>
<div>{printableCoin(props.tx.msgs[0].value.amount, state.chain)}</div>
Copy link
Member

Choose a reason for hiding this comment

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

This is the same issue for the whole file, but we can do

Suggested change
<div>{printableCoin(props.tx.msgs[0].value.amount, state.chain)}</div>
<div>{printableCoin(msg.value.amount, state.chain)}</div>

since we loop over messages already. This makes the code future proof for multi message transactions (which we soon need to withdraw rewards from multiple validators).

) : msg.typeUrl === "/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward" ? (
<>
<li>
<label>Amount:</label>
Copy link
Member

Choose a reason for hiding this comment

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

import StackableContainer from "../layout/StackableContainer";

interface Props {
address: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to make explicit which address this is. For this component we can say delegatorAddress which is then set to the multisig address that signs the transaction.

Copy link
Member

Choose a reason for hiding this comment

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

Also this prop should not be nullable because otherwise we don't create the transaction correctly.

import StackableContainer from "../layout/StackableContainer";

interface Props {
address: string | null;
Copy link
Member

Choose a reason for hiding this comment

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

delegatorAddress as above.

Also this prop should not be nullable.

onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
setValidatorDstAddress(e.target.value)
}
error={addressError}
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need separate error props, one for each field?

!showDelegateTxForm &&
!showUnDelegateTxForm &&
!showRewardsTxForm &&
!showReDelegateTxForm && (
Copy link
Member

Choose a reason for hiding this comment

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

Can you make a separate prop for that (something like anyTxFormVisible) that is automatically updated from those inputs?

import Button from "../../../components/inputs/Button";
import Page from "../../../components/layout/Page";
import StackableContainer from "../../../components/layout/StackableContainer";
import { useAppContext } from "../../../context/AppContext";
import { getMultisigAccount } from "../../../lib/multisigHelpers";

type TxView = "select" | "send" | "delegate" | "undelegate" | "redelegate" | "claimRewards";
Copy link
Member

Choose a reason for hiding this comment

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

Very nice solution! What about chaning "select" to null to make it clear that this is a special case that works differently than the others?

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