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

Support IBC MsgTransfer #128

Merged
merged 15 commits into from
Jun 13, 2023
Merged

Support IBC MsgTransfer #128

merged 15 commits into from
Jun 13, 2023

Conversation

abefernan
Copy link
Contributor

Part of #99.
Closes #127.

@abefernan abefernan self-assigned this Jun 5, 2023
@vercel
Copy link

vercel bot commented Jun 5, 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 Jun 13, 2023 1:33pm

@abefernan abefernan force-pushed the feat/support-ibc-msgtransfer branch from 57ccc9b to e1899eb Compare June 12, 2023 09:08
@abefernan abefernan changed the base branch from master to feat/support-create-vesting June 12, 2023 09:08
@abefernan abefernan marked this pull request as ready for review June 12, 2023 09:08
@abefernan abefernan requested a review from webmaster128 June 12, 2023 09:08
Base automatically changed from feat/support-create-vesting to master June 12, 2023 14:16
types/txMsg.ts Outdated
type OmmitedMsgTransfer = Omit<MsgTransfer, "timeoutHeight" | "memo">;
type MsgTransferRequiredToken = OmmitedMsgTransfer & Required<Pick<OmmitedMsgTransfer, "token">>;
interface MsgTransferOptionalMemo extends MsgTransferRequiredToken {
readonly memo?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep this required and just set it empty when the user does not put in text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was weird that the comment of the type said "optional field" and the field was not optional, but it's not important, I can remove it

Copy link
Member

@webmaster128 webmaster128 Jun 13, 2023

Choose a reason for hiding this comment

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

This is a problem coming from Go and protobuf. There the something is called "optional" wen it is allowed to be empty. Since at transaction level undefined and "" are the same thing we don't need to differentiate it here.

{humanTimestampOptions.map(({ label }) => (
<option key={label}>{label}</option>
))}
</datalist>
Copy link
Member

Choose a reason for hiding this comment

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

I tried this dropdown in the Vercel preview. But it seems like after it was set once you cannot change it anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah it seems it filters the list, maybe I can clear the field on click so all the options appear

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.

Very nice. Let's merge this when you are happy.

The input field is still not creat. Maybe we find a better solution. But let's not block the feature on this.

@abefernan abefernan merged commit e12b360 into master Jun 13, 2023
@abefernan abefernan deleted the feat/support-ibc-msgtransfer branch June 13, 2023 14:19
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.

Add support for IBC transfers
2 participants