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

List LitecoinZ (LTZ) #2221

Merged
merged 1 commit into from
Feb 7, 2019
Merged

List LitecoinZ (LTZ) #2221

merged 1 commit into from
Feb 7, 2019

Conversation

MarkLTZ
Copy link
Contributor

@MarkLTZ MarkLTZ commented Jan 8, 2019

The aim of this PR is to list LitecoinZ (LTZ) on bisq.

This PR replace the previous #1910 closed by @ManfredKarrer

Regards,
Mark

Copy link

@blabno blabno left a comment

Choose a reason for hiding this comment

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

NACK per comments

@@ -147,6 +147,11 @@ private void onSaveNewAccount(PaymentAccount paymentAccount) {
.useIUnderstandButton()
.show();
break;
case "LTZ":
new Popup<>().information(Res.get("account.altcoin.popup.LTZ.msg", "LTZ"))
Copy link

Choose a reason for hiding this comment

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

Does this message bring any added value at this point?
It duplicates validation 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.

If you see messages are quite similar to ZEC but customized for LTZ:

account.altcoin.popup.LTZ.msg=When using {0} you can only use the transparent addresses (starting with L) not the z-addresses (private), because the arbitrator would not be able to verify the transaction with z-addresses.

account.altcoin.popup.ZEC.msg=When using {0} you can only use the transparent addresses (starting with t) not the z-addresses (private), because the arbitrator would not be able to verify the transaction with z-addresses.

Copy link

Choose a reason for hiding this comment

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

I know, but still it doesn't add any value to repeat validation message.
I value your effort with all those translations.
Team will have to discuss if post save popups with this type of messages have any sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well the sense is to advice customers to use only transparent address and not z-address because they are not supported. Now, because LTZ use address starting with 'L' and not with 't' like ZEC does

@ripcurlx
Copy link
Contributor

ripcurlx commented Jan 9, 2019

Just a general comment on changing/adding translations. This only should be done in the English source file (displayStrings.properties) as all other translations are translated within Transifex and will be overwritten/updated before every release.

@MarkLTZ
Copy link
Contributor Author

MarkLTZ commented Jan 9, 2019

@ripcurlx thanks for the info

@blabno
Copy link

blabno commented Jan 22, 2019

Please rebase on top of master and resolve the conflicts.

@MarkLTZ
Copy link
Contributor Author

MarkLTZ commented Jan 22, 2019

Ready in a couple of hours. Hold on

@MarkLTZ
Copy link
Contributor Author

MarkLTZ commented Feb 5, 2019

@blabno done. Please review my changes and merge this PR.
Thanks

@ManfredKarrer ManfredKarrer removed their request for review February 5, 2019 16:01
Copy link

@blabno blabno left a comment

Choose a reason for hiding this comment

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

ACK

Could you squash your commits into a single one?

@ripcurlx
Copy link
Contributor

ripcurlx commented Feb 7, 2019

Also it needs conflict resolution now, because of other merged assets.

@MarkLTZ MarkLTZ requested a review from cbeams as a code owner February 7, 2019 09:33
@blabno
Copy link

blabno commented Feb 7, 2019

rebase and squash into a single commit

@MarkLTZ
Copy link
Contributor Author

MarkLTZ commented Feb 7, 2019

@ripcurlx done

@blabno you can squash all the commits in this pull request into a single commit by selecting "Squash and merge"

@MarkLTZ
Copy link
Contributor Author

MarkLTZ commented Feb 7, 2019

@blabno squashed

@blabno
Copy link

blabno commented Feb 7, 2019

Thank you. Ready for merge.

@ripcurlx ripcurlx merged commit b172c9f into bisq-network:master Feb 7, 2019
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants