mlabs-haskell / nft-marketplace-server

2 stars 2 forks source link

Encode base32 CID into an base36 CID #37

Closed KindaSloth closed 2 years ago

KindaSloth commented 2 years ago

22

CIDS tested: image

CLI encoding: image

Database records with our encoding: image

KindaSloth commented 2 years ago

@samuelWilliams99 about using libraries, I tried a couple of ones, but unfortunately, none works as expected.

And about the other comments, thanks for the review, I'll fix these points.

KindaSloth commented 2 years ago

For some reason in my local machine the "make format" is not working properly, can someone check this?

samuelWilliams99 commented 2 years ago

Formatting fails didnt see your message, will do

t4ccer commented 2 years ago

wrt formatting: https://github.com/mlabs-haskell/nft-marketplace-server/issues/38

KindaSloth commented 2 years ago

@samuelWilliams99 if you can please run the formatter again here (don't know what is happening in my local env)

t4ccer commented 2 years ago

@Guilherme775 What's your issue with formatting? Do you run make format inside nix shell?

samuelWilliams99 commented 2 years ago

The code seems quite involved. Was it not the case that there are just some prefixes in the base32 CID then the rest of the string is just regular base32?

I questioned the lack of external libraries for the logic, apparently they weren't sufficient. Given we're on a bit of a time limit, we can create a ticket to see if we can refactor this to something external later

rynoV commented 2 years ago

We should also somehow make it clear in the signature of this function that it operates on base32 IPFS CIDs and not arbitrary base32 strings

Not sure if this is much better as I just changed most functions to deal with Text instead of CID (I don't have haskell development setup locally so it's a slow process to change types of things, and we're in a bit of a rush). This is a start though, at least the encoding function's type makes it clear now what it operates on. We can improve on this later, possibly in #39

cc @samuelWilliams99