-
Notifications
You must be signed in to change notification settings - Fork 4
feat: support user handles #468
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for dither-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@jeronimoalbi the PR is in draft but you have asked for review. do you need review now? or starting to look at it? |
I guess it can be reviewed anytime if something seems off or can be improved. |
This forces each address to have a single asociated handle. It also keeps queries simple because there is no need to keep track of the current handle if an address would be allowed to have more than one.
Explicit start block value was ignored before.
Removed because a single address can be enough, the one left keeps a bigger history. All in Bits node was configured not track history, so it makes no sense to use it to get previous TXs.
Prepared statements are not useful here because they are used only once. They would make sense where the same query would be executed multiple times.
This makes sure that handle can be transfered to a single address and avoids possible transfer mistakes.
This makes handle available to the profile frontend view.
Display requires a handle to be registered.
Protocol update:
- dither.Register('handle', 'display text')
- dither.Display('display text')
mazzy89
left a comment
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.
Introduce input validation from framework
| export const tgHandleRegex = /^[a-z]{3}\w*$/i; | ||
|
|
||
| export async function Register(body: Posts.RegisterBody) { | ||
| if (!tgHandleRegex.test(body.handle)) { |
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.
I would probably move this input validation to the framework validation. it is way easier to maintain and very iditomatic in a Nodejs framework https://elysiajs.com/essential/validation
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.
Good point, validation should be then done by properly defining constraints within the schema definitions, which is not the case now 🤔
I will take a look, because there might be a reason it has not been done, so far
|
@salmad3 @luciorubeens the last PR adds notification when handle registration fails because is already taken |
packages/api-main/drizzle/schema.ts
Outdated
| export const HandleTable = pgTable( | ||
| 'handle', | ||
| { | ||
| name: varchar({ length: 32 }).primaryKey(), | ||
| address: varchar({ length: 44 }).notNull(), | ||
| display: varchar({ length: 128 }), |
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.
We'll also need to store the profile picture (and possibly other new fields in the future, see #491), so I was thinking we could create an account/profile table instead of one specifically for the handle. Wdyt?
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.
@luciorubeens handle table was renamed to account in 9447752
Protocol related code was refactored to work with the new table in bf956b9
Changes added more complexity to the code but I guess it will make it easier for the new user related features to be implemented.
| return hashes[Math.floor(Math.random() * hashes.length)]; | ||
| } | ||
|
|
||
| // TODO: Add support for user handle (registerHandle, transferHandle, acceptHandle, displayHandle) |
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.
Could you implement this? So we will be able to test it locally.
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.
Maybe it would be better to add in in a different PR, to make reviews here easier, WDYT?
The network spammer could be easily replaced by custom send TX with the specific features to test.
Done for future compatibility with user profiles.
Handle table was replaced by account table to make it easier for new upcoming user related features to be implemented.
Previous configuration was reloading but updates were not recognized.
This is a temporary solution to be able to see the unshortened user address, handle or display.
Handles are now part of a generic account table.
Size is long enough and it fits the UI without needing to shorten it.
Composable gets user account info from the API.
Validates handle format, length and availability.
Done to avoid query framework to keep making retry requests.
|
We should probably consider to have a minimum PHOTON amount required when registering a handle |
Fee must be configured for the reader and also the frontend.
|
PR requires support of an anti-squatting system before merge |
Related to #411