-
Notifications
You must be signed in to change notification settings - Fork 353
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
Add TokensByOwner for cw721-base #122
Conversation
2e034ea
to
6614068
Compare
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.
Looks good. I think the use of bounds is still a little bit confusing / needlessly complex.
Added some comments to try and help clarifying the design.
My takeaways. Only use one type:
and accept This will lead to lines like: `let start: Option = start_after.map(|human| Ok(Bound::Exclusive(deps.api.canonical_addr(&human)?)).transpose()?; Which is not so legible or user-friendly. I guess having a function for handling the
or
both of which are simple enough, without all these helpers. Does the above make sense to you? Other suggestion? |
Makes sense. Will review later again with the IDE, to see and try some variants. |
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 understand this is WIP, but what I see looks good to me.
Powerful stuff here, and in the storage-plus.
b989e6d
to
ae42b7d
Compare
ae42b7d
to
6465625
Compare
Closes #81
Builds on #108 (merge that first)
Ports to use
cw-storage-plus
rather thancosmwasm-storage
, then makes use of IndexedMapcw-storage-plus
IndexedMap