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

Added Citadel gateway #1695

Merged
merged 20 commits into from
Sep 6, 2018

Conversation

snakebitekit
Copy link
Contributor

@snakebitekit snakebitekit commented Jul 13, 2018

Hi, it's citadel gateway which supports citadel.monero tokens and monero(xmr) coins.
Both main and beta deposit&withdraw interfaces are tested and working, you can check it on our prod server: https://citadel.li/wallet/#/deposit-withdraw

There's one more question by the way: can i add a citadel's help block in this pr or should i do it in another?

@sschiessl-bcp
Copy link
Contributor

We will get back to you on this pull request.

Your link above seems to not work anymore. I assume you only added into the modal? It also is shown as disabled right now.

@snakebitekit
Copy link
Contributor Author

@sschiessl-bcp hm, can u try to log in and recheck it?
because on my side everything is ok:
screen shot 2018-07-20 at 19 41 47

Copy link
Contributor

@startailcoon startailcoon left a comment

Choose a reason for hiding this comment

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

Assets aliases should be handled with code from the API Settings, rather than within the code directly.

? selectedGateway.toLowerCase() + "." + assetName
: assetName;
if (
assetName.toLowerCase() === "monero" &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this change as the new modals aim to be generalized for making it easier to add new gateways. I would rather have this done on a per gateway in the settings file.

Something like
assetWithdrawlAlias: { monero: xmr }

@@ -276,6 +276,12 @@ export function requestDepositAddress({
url = openledgerAPIs.BASE,
stateCallback
}) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be handled in a gateway specific setting

@snakebitekit
Copy link
Contributor Author

@startailcoon ty for your correction
i've made a commit to handle this: 7213e01

@snakebitekit
Copy link
Contributor Author

snakebitekit commented Jul 31, 2018

@startailcoon @sschiessl-bcp does it look fine or do i need to fix something else?

@startailcoon
Copy link
Contributor

@snakebitekit I can do a review of this in the coming days

@startailcoon startailcoon self-requested a review August 1, 2018 08:58
@snakebitekit
Copy link
Contributor Author

@startailcoon great, thanks!

@snakebitekit
Copy link
Contributor Author

snakebitekit commented Aug 7, 2018

hi, @startailcoon
how is it going?

@startailcoon
Copy link
Contributor

I've had issues with my developer computer setup, which made me delayed in the process. You're not forgotten and I will look at this as soon as possible.

@snakebitekit
Copy link
Contributor Author

@startailcoon ok, thanks for reply.
we're really looking forward to it.
if you need some tokens or transaction history to prove it – i'll send it back to you, just ask)

@startailcoon
Copy link
Contributor

Some tokens would be good to be able to make some test transactions.
You can send them to account startail-test

@startailcoon
Copy link
Contributor

I've detected 1 issue and a few changes that I will submit when I've finished testing. It does look good and works well.

I've looked trough the functions of the legacy Deposit/Withdraw function, which I'll test as soon as I have some tokens.

One issue is the Modal that will require some layout fixes to support some issues with the text being to long as well as the address being very very long.

An example is the deposit address I receive: 4GvPuQofExxWapNcJvj6yU1oYhLXm9rPvRgH9TubBUej1Z44c7QatQABMsaAx1hWYHMiMHHtaYYGPjVtt1iHLSsL9zXJhdgeP7876CECi1

We will need to wrap this elegantly, without making it break something else.

image

@sschiessl-bcp
Copy link
Contributor

I've looked trough the functions of the legacy Deposit/Withdraw function, which I'll test as soon as I have some tokens.

When will it be legacy? Or receive a make-over to stick to the logic from the modal?

@startailcoon
Copy link
Contributor

startailcoon commented Aug 7, 2018

When will it be legacy? Or receive a make-over to stick to the logic from the modal?

I'm not sure, but we've talked about doing something about it. They are more Gateway specific, and could probably stay in one form or another. They provide some more space for gateway specific features and functions, like updates, news, contact details etc. I see these sections more of a space where the gateways can and should take care of their own code, push PRs when they want an update etc.

Further discussion about Withdraw/Deposit section is on issue #1479

@snakebitekit
Copy link
Contributor Author

@startailcoon we've sent some citadel.monero tokens to startail-test account.

- Handle longer addresses on Deposit + Confirm
- Fix 'assetWithdrawlAlias' when gateway is missing option
- Various minor style fixes
@startailcoon
Copy link
Contributor

I've supplied a commit with some changes to fix some issues with this PR.
Overall this looks good and it works well when I've tested it.

I can't confirm the Deposit feature. I tried to withdraw 0.1 CITADEL.MONERO to the assigned deposit address, but it didn't show up.

Copy link
Contributor

@startailcoon startailcoon left a comment

Choose a reason for hiding this comment

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

Supplied commit fixes the assetWithdrawlAlias issue with gateways that had no alias params.

- Fixes long deposit address on DepositWithdraw component
@startailcoon
Copy link
Contributor

startailcoon commented Aug 9, 2018

Additionally you should commit a document named citadel.md to the app\help\en\gateways folder to view for users that click the question mark on deposit and withdraw modal.

Link this document to the toc.md document. This will then be viewed in the help section under gateways.

@snakebitekit
Copy link
Contributor Author

@startailcoon okay, i'll add a help block and commit it.
can we chat directly in messenger to debug more fluently? telegram\discord etc..
i've found you on discord by the way, is it comfortable way to chat for you?

Copy link
Contributor

@startailcoon startailcoon left a comment

Choose a reason for hiding this comment

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

Also need to add word-break to Transactions memo as the address is so long.

<td className="memo" style={{wordBreak: "break-all"}}>{text}</td>

On

<td className="memo">{text}</td>

this.props.asset.get("id")
)
) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail when amount_to_withdraw is null or undefined.

Please add the following

!!this.props.amount_to_withdraw &&

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ty, is it ok now 99974bd ?

@snakebitekit
Copy link
Contributor Author

sup?

app/api/apiConfig.js Outdated Show resolved Hide resolved
app/api/apiConfig.js Outdated Show resolved Hide resolved
@sschiessl-bcp sschiessl-bcp merged commit dd114f8 into bitshares:develop Sep 6, 2018
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