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

v0.45.1 support for /ununifi/cdp and /unifi/cdp/create #229

Merged
merged 11 commits into from
Mar 29, 2022

Conversation

taiki1frsh
Copy link
Contributor

The problem was the change of the type that is created by ununifi-client automatically.

Specifically, IParams became to have two array objects which are collateral_params and debt_params. But, the code wasn't applied to these change.

So, we made principalDebtParam variable to compromise with pages/create/create component com in project/telescope-extension/src/app/utils/function.ts.

@taiki1frsh taiki1frsh changed the title Feat/version v0.45.1 cdp create [WIP]Feat/version v0.45.1 cdp create Mar 28, 2022
@taiki1frsh
Copy link
Contributor Author

After some more revision of the other component to make build pass, I tried to create a CDP with type ubtc-jpy-3-2.

But, the same issue that we (me and @YasunoriMATSUOKA ) had occurred.
So, I look into this component again.

@YasunoriMATSUOKA
Copy link
Contributor

@taikiFurusyo
Thank you for your reporting. Please continue to investigate them again.

Well, I concern that the reason CI is failing is because the changes to package-lock.json have not been committed. Please execute npm i again and commit the changes to package-lock.json, just to be sure.

@taiki1frsh
Copy link
Contributor Author

Oh, thanks for the precise advise.
In local, the build failed with the branch feat/v0.45.1-cdp's issue anyway, but it is so helpful.

@Senna46
Copy link
Contributor

Senna46 commented Mar 29, 2022

ff3e998
The contents of ununifi.cdp.IParams retrieved are now compatible with v0.45.1.
But, the number of parameters that can be retrieved has changed from one to two, and the display is now corrupted. This will be corrected as well.
screencapture-localhost-4200-ununifi-cdp-2022-03-29-15_44_09

@YasunoriMATSUOKA
Copy link
Contributor

@Senna46
OK. Thank you.

@Senna46
Copy link
Contributor

Senna46 commented Mar 29, 2022

@YasunoriMATSUOKA
I think the build will go through without problems.
Please review /ununifi/cdp and /unifi/cdp/create, if you have no problems.

Changes

The display of CDP parameters is changed.

  • The part that used to be Other Status is now included in Debt Params, so it is displayed according to Debt Params.
  • The display of the parameters in *ngFor, which was not beautiful, has been improved by displaying the type for Collateral Params and the denom for Debt Params outside the card.]

screencapture-localhost-4200-ununifi-cdp-2022-03-29-16_07_07

@YasunoriMATSUOKA
Copy link
Contributor

@Senna46
Can you create cdp with this PR implementation?

@Senna46 Senna46 changed the title [WIP]Feat/version v0.45.1 cdp create v0.45.1 support for /ununifi/cdp and /unifi/cdp/create Mar 29, 2022
@Senna46
Copy link
Contributor

Senna46 commented Mar 29, 2022

Untested. is it ok to check with ununifi-8-private-test?

@YasunoriMATSUOKA
Copy link
Contributor

Yes. Plz check with ununifi-8-private-test.

@Senna46
Copy link
Contributor

Senna46 commented Mar 29, 2022

@YasunoriMATSUOKA CC: @taikiFurusyo
After some modifications to the code, CDP creation is now possible.
The CDP has been created as shown in the second photo.

On the other hand, there is a problem that it does not appear in /cdp/cdps.
I will continue working on this, but if you are in a hurry, you can merge this and deal with it in another Pull Request.

スクリーンショット 2022-03-29 170218
スクリーンショット 2022-03-29 170332
スクリーンショット 2022-03-29 170355

@YasunoriMATSUOKA
Copy link
Contributor

@Senna46
Thank you for your fix!
But, on the following screenshot, created cdp should be shown in the list, but it is not shown in the list.
Can you fix that?
image

@Senna46
Copy link
Contributor

Senna46 commented Mar 29, 2022

@YasunoriMATSUOKA
As mentioned above, this will be a /cdp/cdps problem, should this Pull Request solve the problem?
I will proceed with the work.

@YasunoriMATSUOKA
Copy link
Contributor

@Senna46
Thank you for your comment.
Sorry, I missed your comment.
Let's merge it once and fix it in the next PR.

Copy link
Contributor

@YasunoriMATSUOKA YasunoriMATSUOKA left a comment

Choose a reason for hiding this comment

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

LGTM

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