Page MenuHomePhabricator

[Cashtab] [Alias] pt 1 - Create scaffold for new Alias component
ClosedPublic

Authored by emack on Dec 11 2022, 01:36.

Details

Summary

T2551

Adds a new Alias component to house the UI for the namespace management function.

UI scaffolding, pseudocode and snapshot in this diff only - no functionality added yet.

  • Kieran's building the CustomIcon update for the Alias navigation, will revise this part 1 once ready.

Implementation Plan
Implement registerNewAlias() to handle alias registration
[Cashtab] [Alias] pt 2 - Implement registerNewAlias() to handle alias registration
[Cashtab] [Alias] pt 3 - Implement isAliasAvailable function
[Cashtab] [Alias] pt 4 - Implement isAddressRegistered function
[Cashtab] [Alias] pt 5 - Implement getAddressFromAlias function
[Cashtab] [Alias] pt 6 - Implement isLocalAliasStateLatest function
[Cashtab] [Alias] pt 7 - Enable alias lookup for Send XEC component
[Cashtab] [Alias] pt 8 - Enable alias lookup for Send Token component
[Cashtab] [Alias] pt 9 - Implement pricing mechanism
[Cashtab] [Alias] pt 10 - Server cron job
[Cashtab] [Alias] pt 11 - Upgrade tx history to recognize alias registration txs

Test Plan

npm test
npm start
click on hamburger menu and navigate to Alias
review the UI, icons, labels, inputs
review the pseudocode starting in Alias.js, and touching cashMethods, transactions...etc and verify our brainwaves are aligned

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Errors
SeverityLocationCodeMessage
Errorweb/cashtab/src/utils/chronik.js:471ESLINTetc/no-commented-out-code
Errorweb/cashtab/src/utils/transactions.js:26ESLINTetc/no-commented-out-code
Unit
No Test Coverage
Build Status
Buildable 21686
Build 43012: Build Diffcashtab-tests
Build 43011: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
emack requested review of this revision.Dec 11 2022, 01:36
emack edited the summary of this revision. (Show Details)
bytesofman added inline comments.
web/cashtab/src/components/App.js
23 ↗(On Diff #37193)

Whatever is done for App.js should also be done for extension/src/components/App.js

516 ↗(On Diff #37193)

If passLoadingStatus is necessary, it should be provided here

If not necessary, should be removed from the Identity component

583 ↗(On Diff #37193)

Might as well give it a good placeholder icon here, at least something that is unique.

<CheckCircleOutlined /> from antd default library would work. Open to other suggestions.

https://ant.design/components/icon

web/cashtab/src/components/Common/Ticker.js
54 ↗(On Diff #37193)

This should be added to /web/standards/op_return-prefix-guideline.md in the same diff

This revision now requires changes to proceed.Dec 11 2022, 03:53
emack marked 4 inline comments as done.Dec 11 2022, 05:48
emack added inline comments.
web/cashtab/src/components/App.js
23 ↗(On Diff #37193)

ported changes to extension

516 ↗(On Diff #37193)

it would be necessary as the registration process will be async

583 ↗(On Diff #37193)

Opting for <UserOutlined /> as it better represents the identity component.

emack marked 3 inline comments as done.
  • ported Identity scaffold to cashtab extension
  • added passLoadingStatus to Identity route due to alias registration being async
  • added proposed namespace prefix to OP_RETURN Prefix Guideline
  • Using antd's <UserOutlined /> as temporary placeholder, sizing/positioning will be adjusted once it's imported via CustomIcons

We probably don't want to call the component Identity since it has nothing to do with kyc or identity. Alias.js is probably better, or AliasRegistration.js, depending on your vision for the future specific use of this screen.

web/cashtab/src/components/Identity/Identity.js
50 ↗(On Diff #37194)

Instead of setting it to be a string, it should be initialized as false or null. Then there should be conditional rendering on what to say if alias is false or null.

In this case, null probably makes the most sense. When the page loads, the app doesn't know if the user will have an alias or not. After Cashtab figures this out, perhaps alias is set to false or true (if this is simply being used as a boolean...in which case mb something like hasValidAlias would be a better name). Or it becomes an array, either empty if no valid aliases or populated if the address has valid aliases registered.

Cashtab has been a bit sloppy in implementing this stuff properly from the get go in the past. We should put some thought into how this variable will be used.

59 ↗(On Diff #37194)

Since this will be a standard, no need for us to put it in all the registering transactions. Limits the namespace available.

Also, maybe we don't want the standard to be "all aliases end with .xec, or maybe it changes someday.

Alias registrations should be just the desired alias.

65 ↗(On Diff #37194)

This will depend on the character length of the desired alias. So, we will probably want to keep this as an object or array in the currency object in Ticker.js

72 ↗(On Diff #37194)

We won't really know if it's available until at least 1 conf on the tx. So will still need to handle a pending state.

84 ↗(On Diff #37194)

Can filter this further with "for each incoming tx with OP_RETURN inputs that paid at least minFee"

140 ↗(On Diff #37194)

In theory, a single address could have multiple valid aliases.

We could make some kind of rule to prevent this, but I don't think we should. Mb someone wants 10 aliases for the same address, or someone is speculating on the value of alias NFTs in the future.

145 ↗(On Diff #37194)

see above. I think it's more work to exclude the possibility of multiple aliases for one address than it is to simply allow it. Some users might want it.

Every alias must match to one and only one address.

But it's okay for an address to match to many aliases.

web/cashtab/src/utils/transactions.js
27 ↗(On Diff #37194)

This should be handled in the background of the Send.js component before the "send tx" button is enabled. So, we do need this function, but I don't think transactions.js is the right place for it.

32 ↗(On Diff #37194)

This is a novel way to save time in this function. Because it's also used in other alias verification functions, it should be its own function, something like isLocalAliasStateLatest

41 ↗(On Diff #37194)

In the (unlikely but possible) event that firstTimeSeen is lower for one alias registration tx, but a later transaction for the same alias confirmed sooner, we want to defer to the first confirmed and not the first seen.

This revision now requires changes to proceed.Dec 11 2022, 15:12
web/standards/op_return-prefix-guideline.md
21 ↗(On Diff #37194)

How about 2E786563 for .xec ?

We're not looking to create something exclusive to Cashtab

emack retitled this revision from [Cashtab] [Identity] pt 1 - Create scaffold for new Identity component to [Cashtab] [Alias] pt 1 - Create scaffold for new Alias component.Dec 12 2022, 04:00
emack edited the summary of this revision. (Show Details)
emack edited the test plan for this revision. (Show Details)
emack marked 11 inline comments as done.
  • Changed Identity references to Alias (verified via grep)
  • No longer appending '.xec' as part of the namespace value being committed onchain. This will be handled in parts 6 & 7 when parsing, xec aliases for sending XEC/tokens.
  • Added aliasSettings object in Ticket.js, including aliasRegistrationFeeInSats array containing the 8 different pricing tiers depending on the number of chars in the alias.
  • Added pending state pseudocode placeholder in registerAlias()
  • Added currency.aliasSettings.aliasMinFee as additional filtering criteria
  • Alias rendering assumptions adjusted so the Alias state variable is an array, users can register one or more aliases for one address and the registration input will not be conditionally hidden
  • getAddressFromAlias moved from Transactions.js to chronik.js rather than Send.js per feedback because this needs to be called from sendToken as well, so needs to be accessible by both. It's appropriate for chronik because it will need to make a chronik tx history call.
  • Added isLocalAliasStateLatest as a new function in transactions.js to handle caching triggers
  • Updated alias prefix to 2E786563 in both Ticket.js and OP_RETURN prefix standard
  • Updated Alias.test.js snapshot

Incorporated @kieran709's custom icon from D12872 for the alias navitem. No further outstanding components for this part 1.

bytesofman added inline comments.
web/cashtab/src/components/Common/Ticker.js
12 ↗(On Diff #37250)

Once we set these rates, it will be pretty much impossible to change them.

Also, we probably will not want to test with whatever final prices are.

So, we should have something here like

{
oneChar: 150,
twoChar: 125,
threeChar: 100,
fourChar: 75,
fiveChar: 50,
sixChar: 25,
sevenChar: 20,
eightChar: 15,
minFee: 10,
}

In this way, we can write code that parses for variables like aliasRegistrationFeeInSats.oneChar -- test with a lower fee table than what goes into prod, but not have to mess with the functions to update pricing before deployment.

This revision now requires changes to proceed.Dec 13 2022, 18:55
emack marked an inline comment as done.

Added nominal registration pricing tiers for dev purposes only

bytesofman added inline comments.
web/cashtab/src/components/Common/Ticker.js
14 ↗(On Diff #37269)

Initially, I was thinking about validating aliases based on pricePaid > appropriateFee

However, now I think it would be better (and easier) to just use fixed prices. This will also make testing easier.

This way, to start with, everything has a set price. So, a tx that pays for a 3-char alias is only valid if the paid fee (to the proper address) exactly equals the price, say 1000.

If we change prices at some future date, then we validate by "paid fee exactly equals newPrice && blockHeight > cutOffHeightWhereNewPricesImplemented"

Better to use exact prices to validate instead of ranges, because then we can also automate the handling of edge cases that are fun to think should not exist but always come up somehow (e.g. , someone pays way too much money for a short alias, system says its valid and they get the alias, but really they want a refund bc they overpaid).

also this makes testing easier and cheaper, since we can just use prices like

aliasRegistrationFeeInSats: {
oneChar: 558,
            twoChar: 557,
            threeChar: 556,
            fourChar: 555,
            fiveChar: 554,
            sixChar: 553,
            sevenChar: 552,
            eightChar: 551,
            minFee: 550, // dust
}

Then the backend refunds all invalid registrations on a chron job (if this gets to be a big enough problem to require automation. I imagine, at first, any necessary refunds may be processed manually. Depending on how things go we may not want the receiving wallet to be a hot wallet).

This revision is now accepted and ready to land.Dec 15 2022, 00:20

updated nominal pricing tiers

cleanup to comply with new eslint policies