-
Notifications
You must be signed in to change notification settings - Fork 90
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
frontend/coin-control: add address reuse warning #2724
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just noticed a small bug.
I also feel like the duplicated address should be somehow highlighted. Otherwise it can be hard to find those out.
{utxo.address} | ||
</span> | ||
<div> | ||
<Status hidden={!hasAddressReuse} type="warning" className="m-bottom-default"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bebb863
to
c3a2cd2
Compare
PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -56,6 +58,7 @@ export const UTXOs = ({ | |||
const { t } = useTranslation(); | |||
const [utxos, setUtxos] = useState<TUTXO[]>([]); | |||
const [selectedUTXOs, setSelectedUTXOs] = useState<TSelectedUTXOs>({}); | |||
const [showAddressReusedMessage, setShowAddressReusedMessage] = useState(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I find the name of this const slightly misleading. Maybe smth like selectedReusedAddresses
? Since it is a number and not a bool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to reusedAddressUTXOs
.
I'll take a look, ideally badge would be nice and inline with text. Not sure yet what causes this. |
c4b34db
to
dc0e54e
Compare
tl;dr it was a combination of inconsitent line-heights (parent element) and vertical centering. branch with fix @strmci @Beerosagos let me know if you want to merge this PR first and if I should open a followup PR or feel free to chery-pick my commit into this change. |
@thisconnect |
Add a warning to the coin control modal when a user selects one or more UTXOs from reused addresses. The warning: "One or more UTXOs have a reused address. Be aware the receiver will see all UTXOs associated with those addresses." The UTXOs now have red badge when their address was re-used.
Badges are not vertically aligned consistently in all places. This was caused mostly as badges were centerd vertically but then had slightly different line-height. Badges behave like text and usually have `display: inline;`. Changed line-height to be consistent and vertical alignment to baseline, so that the badge text stands on the same line as the other text. Badges currently appear in the following components: - account selector can have badge with icon only - sidebar shows badge for connected keystore with icon and text - reused utxo's in coincontrol have badge with warning text - account summary balance have badge for connected keystore - exchange options show best-deal or fast badge - manage-accounts shows badge for connected keystore
Add a warning to the coin control modal when a user selects one or more UTXOs from reused addresses. The warning: "One or more UTXOs have a reused address. Be aware the receiver will see all UTXOs associated with those addresses."
The UTXOs now have red badge when their address was re-used.