-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add nft holder endpoints #743
Conversation
lib/ae_mdw/aex141.ex
Outdated
def fetch_owned_nfts(state, account_pk, cursor, pagination) do | ||
case deserialize_cursor(cursor) do | ||
{:ok, cursor_key} -> | ||
{prev_cursor_key, nfts, next_cursor_key_tuple} = |
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.
Why is one called _key
and the other one _tuple
? I'd rather describe values by what they represent (key) instead of what they are (tuple)
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.
the tuple one is the cursor used by PaginatedPlug
with the is_reversed?
check
@@ -68,6 +72,8 @@ defmodule AeMdw.Error do | |||
defexception!(NotFound) | |||
defexception!(Expired) | |||
defexception!(NotAex9) | |||
defexception!(NotAex141) | |||
defexception!(ContractReturn) |
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.
🙏
@spec nft_owner(Conn.t(), map()) :: Conn.t() | ||
def nft_owner(conn, %{"contract_id" => contract_id, "token_id" => token_id}) do | ||
with {:ok, contract_pk} <- Validate.id(contract_id, [:contract_pubkey]), | ||
{token_id, ""} <- Integer.parse(token_id), |
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.
This is going to return :error
which is not handled by the FallbackController
.
I'd perhaps send the contract and token ID directly to Aex141.fetch_nft_owner/2
function and let it return an error if any of these things are invalid
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.
Done here in the controller because call_contract
also returns :error
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.
This way endpoint params validation can be done also closer to border (entry door)
lib/ae_mdw_web/router.ex
Outdated
{"/aex141", AeMdwWeb.AexnTokenController, :aex141_tokens}, | ||
{"/aex141/:contract_id", AeMdwWeb.AexnTokenController, :aex141_token}, | ||
{"/aex141/:contract_id/owner/:token_id", AeMdwWeb.Aex141Controller, :nft_owner}, | ||
{"/aex141/owned_nfts/:account_id", AeMdwWeb.Aex141Controller, :owned_nfts} |
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.
Like with account-balances
down below, I think we should make all of our routes use -
instead of _
which is the friendly URL way of formatting it
lib/ae_mdw/aex141.ex
Outdated
def fetch_owned_nfts(state, account_pk, cursor, pagination) do | ||
case deserialize_cursor(cursor) do | ||
{:ok, cursor_key} -> | ||
{prev_cursor_key, nfts, next_cursor_key_tuple} = |
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.
Both prev and next return the {key(), is_reversed?()}
tuple, so whatever we name them they should be the same
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.
It's true, it was a misleading by this:
ae_mdw/lib/ae_mdw/collection.ex
Line 44 in ec2095d
{records, [next_cursor]} -> {prev_cursor, records, {next_cursor, false}} |
but the serialization is the same
refs #629