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

Manage assets #412

Merged
merged 19 commits into from
Mar 29, 2022
Merged

Manage assets #412

merged 19 commits into from
Mar 29, 2022

Conversation

piyalbasu
Copy link
Contributor

Screen.Recording.2022-03-28.at.8.21.45.PM.mov

@github-actions
Copy link
Contributor

balances: Balances;
networkDetails: NetworkDetails;
}) => getAssetIconsService({ balances, networkDetails }),
);
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'm adding assetIcons to the redux store because we always fetch all these icons when we land on Account, and we need them for subsequent views like Manage Assets and Send Payment. I figure we can save ourselves reaching back out for toml files by saving the url's in redux for reuse

Ideally, this will hold an asset's home domain, as well, at some point

@github-actions
Copy link
Contributor


const errorState: TRUSTLINE_ERROR_STATES = error
? mapErrorToErrorState(getResultCode(error))
: TRUSTLINE_ERROR_STATES.UNKOWN_ERROR;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse the result_code we get back from Horizon and map it to a Error UI

@piyalbasu
Copy link
Contributor Author

The UX is still WIP, so I tried not to go too deep until we finalize some details

Copy link
Contributor

@acharb acharb left a comment

Choose a reason for hiding this comment

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

💯 left some comments, mostly little things

setErrorAsset: (errorAsset: string) => void;
}

const getCodeIssuerStr = (assetCode: string, assetIssuer: string) =>
Copy link
Contributor

@acharb acharb Mar 29, 2022

Choose a reason for hiding this comment

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

❓ can we call this getCanonicalFromAsset or something similar? there's the opposite func in the helpers now, could we move this one next to it in that file? (I'm sure we'll re-use this at some point)

I've been using the term canonical when referring to <asset code>:<issuer>, I've noticed we're not 100% consistent referring to it that way across projects but I think it's a good way to refer to it, and is used in SEPs (though not all idt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good call

import "./styles.scss";

export enum TRUSTLINE_ERROR_STATES {
UNKOWN_ERROR = "UNKNOWN_ERROR",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
UNKOWN_ERROR = "UNKNOWN_ERROR",
UNKNOWN_ERROR = "UNKNOWN_ERROR",

errorMessage: string;
response: Horizon.TransactionResponse;
};
rejectValue: ErrorMessage;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, thanks 😄

@@ -72,17 +74,10 @@ export const Account = () => {
});
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ can you use the new sortBalances helper in helpers/account.ts to replace the lines above? (line 68-73)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, yes, good catch!


const { balances } = accountBalances;

if (!balances) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we also disable the ManageAssets button if no assets? otherwise it will just load this view and then redirect back (or not sure if you were thinking of doing something different for next PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do actually hide the button based on the isFunded which, now that I'm thinking about it, means the same thing as !balances. I'll create a ticket to address that as isFunded shouldn't be needed

@github-actions
Copy link
Contributor

Copy link
Contributor

@acharb acharb left a comment

Choose a reason for hiding this comment

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

lgtm!

@piyalbasu piyalbasu merged commit edb6b20 into release/2.0.0 Mar 29, 2022
@piyalbasu piyalbasu deleted the manage-assets branch March 29, 2022 20:34
piyalbasu added a commit that referenced this pull request Apr 26, 2022
* add Account History detail view

* only show From if it exists

* label updates

* add add/manage assets views

* consolidating asset icon logic

* undo get asset icons consolidation

* finish modify trustline and add error screens

* show existing balance during error

* handle error cases for XLM

* handle possible errors

* set tx timeout

* pr comments
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.

2 participants