Skip to content

watchlist worker #227

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

watchlist worker #227

wants to merge 13 commits into from

Conversation

newraina
Copy link
Member

@newraina newraina commented Dec 9, 2023

Ref: #64

test: watchlist.0xxmmvv.workers.dev

APIs:

  • GET /watchlists - list all watchlists
  • POST /watchlists - create watchlist
  • GET /watchlists/:id - get one watchlist
  • PUT /watchlists/:id - update one watchlist (only name can be updated for now)
  • DELETE /watchlists/:id - delete watchlist
  • GET /watchlists/:id/chains/:chain_id/items - list items in watchlist
  • POST /watchlists/:id/chains/:chain_id/items - add to watchlist
  • GET /watchlists/:id/chains/:chain_id/items/:item_id/exists - check if item exists in watchlist
  • DELETE /watchlists/:id/chains/:chain_id/items/:item_id - remove from watchlist

request headers:

  • x-auth-address address of user

@vikiival
Copy link
Member

vikiival commented Dec 9, 2023

Please follow README.md

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

Does not follow contributing standards

@newraina
Copy link
Member Author

newraina commented Dec 9, 2023

Please follow README.md

@vikiival added the kysely-codegen part and reorganized routes by files.

@newraina newraina requested a review from vikiival December 12, 2023 00:27
Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

~

@vikiival
Copy link
Member

I checked the ref issues and it not written correctly:

Please let me know if something is unclear for you, I can expand

@newraina
Copy link
Member Author

I checked the ref issues and it not written correctly:

Please let me know if something is unclear for you, I can expand

Thanks! will fix them in this week

@newraina
Copy link
Member Author

@vikiival I've overhauled all APIs using a two-entity approach. Given our watchlist UI currently doesn't support multiple watchlists, I introduced a 'default' watchlist for simplicity.

While it may seem like I've created many APIs, only a handful will be used in our UI's initial version:

  • GET /watchlists/default/chains/:chain_id/items - list items in default watchlist
  • POST /watchlists/default/chains/:chain_id/items - add to default watchlist
  • DELETE /watchlists/default/chains/:chain_id/items/:item_id - remove from default watchlist
  • GET /watchlists/default/chains/:chain_id/items/:item_id/exists - check if item exists in default watchlist

@newraina newraina requested a review from vikiival January 28, 2024 03:52
Comment on lines 3 to 10
export function isValidAddress(address: string) {
try {
decodeAddress(address)
return true
} catch (e) {
return false
}
}
Copy link
Member

Choose a reason for hiding this comment

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

try is adress valid from the same package ;))

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to use isAddress

Copy link
Member

@vikiival vikiival left a comment

Choose a reason for hiding this comment

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

The main problem I have is the default watchlist,

the name of default should be the same as the address :).

and therefore you do not need a ton of checks

newraina and others added 2 commits January 29, 2024 19:59
Co-authored-by: Viki Val <viktorko99@gmail.com>
@newraina
Copy link
Member Author

newraina commented Jan 29, 2024

the name of default should be the same as the address, and therefore you do not need a ton of checks

@vikiival Could you clarify this for me? Sorry I'm a bit confused how does setting the default watchlist's name as 'address' reduce the number of checks we need to perform?

@vikiival
Copy link
Member

Could you clarify this for me?

there should be no routes with 'default' watchlist. Default watchlist for user should have name = address

E.g. My default watchlist should be 5FFG7GHAR2F2xCb5bqaLQrdgNGdtEL64tYgZimRBHDjuxVuA

reduce the number of checks we need to perform

You are doing a magic in the code to 'handle' is default :)

@newraina
Copy link
Member Author

@vikiival Got it, how about this approach:

The ID of default watchlist = address, rather than the name = address. This means I won't have to look up the ID by name every time I get URLs like /watchlists/5FFG7GHAR2F2x.../*. It helps me avoid having to write code like the example below:

if (req.params.watchlistId === address) {
  // it's user's default watchlist
  const id = findWatchlistIdByName(address)
  ...
}

@vikiival
Copy link
Member

vikiival commented Apr 6, 2024

Sounds good are you planning to continue on this one @newraina ?

@newraina
Copy link
Member Author

newraina commented Apr 6, 2024

@vikiival sure, I can continue in next week

@newraina newraina requested a review from vikiival April 12, 2024 13:56
@newraina
Copy link
Member Author

@vikiival removed all routes associated with the 'default' watchlist. To achieve this, I introduced a new publicId field for the watchlist. The publicId for user-created watchlists is a random string, while the publicId for a user's default watchlist is their address.

publicId will appear in the URL as the identifier ID for the watchlist: GET /watchlists/:public_id/.... Primary key id in the database will be only used to associate watchlist items and not be exposed to the user.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants