Skip to content

Conversation

@pandablue0809
Copy link
Contributor

Issue

NFT mint (XRPL)

Discription

XRPL-NFT-Mint-04-30-2025_11_45_PM

@ihomp
Copy link
Member

ihomp commented May 1, 2025

  • the tabs [XRPL | Xahau] looks good, but it's missing all the test/dev networks
    For example, on tha page https://xahauexplorer.com/en/faucet - we have listed all the networks

We have the network change in the top right corner.
so, I'm not sure if we should add all the test networks in the tabs as well, or shall we just remove the tabs... as there is a switcher in the right corner.

What are your thoughts on this? @pandablue0809 @Anna15170221

@ihomp
Copy link
Member

ihomp commented May 1, 2025

@pandablue0809 I see you have "Add digest" for the XRPL. I thought Digest was only available for uritokens on Xahau. Does it do anything for the XRPL transaction? if not, that can be removed from XRPL.

@ihomp
Copy link
Member

ihomp commented May 1, 2025

@Anna15170221, please create a separate ticket to make "issuer" inputs search only through NFT issuer addresses. @ildaruz can provide the endpoint and examples for the query.
That would need to be updated here and in all the other places where an NFT issuer is searched/typed in.

@ihomp
Copy link
Member

ihomp commented May 1, 2025

Issuer (optional - if different from your account):

That might look a bit confusing for newcomers.
From UI it would be better to do it in the way we did it for Digest in Xahau.

a checkbox like
Mint on behalf of another account

And when it's checked, show the field
Issuer

@ihomp
Copy link
Member

ihomp commented May 1, 2025

I see Expiration on the screenshot, but I don't see it the page now.
It's ok to do it without Expiration at the moment; the expiration can be added later in a separate PR.

@ihomp
Copy link
Member

ihomp commented May 1, 2025

I think we also need here a checkbox like

Create a Sell offer
When checked, then three new fields will be shown:

  • Amount
  • Destination
  • Expiration (it's ok to have it in a separate PR later)

Thi all Sell offer functionality can be added in a separate PR later, if you want to make the first PR simpler.

@pandablue0809
Copy link
Contributor Author

hi @ihomp
I fixed
About Expiration and Create a Sell offer I will implement in next PR
Plz check

@ihomp
Copy link
Member

ihomp commented May 3, 2025

@pandablue0809

The created component NftMintTabs is basically a copy of the existing one, FaucetTabs
It would be nice to reuse the same component for easier maintenance.

  • remove NftMintTabs
  • rename FaucetTabs to NetwortkTabs (do a bit of renaming inside of the component)
  • replace the use of FaucetTabs and NftMintTabs to NetwortkTabs

@ihomp
Copy link
Member

ihomp commented May 3, 2025

hi @ihomp I fixed About Expiration and Create a Sell offer I will implement in next PR Plz check

Oh, I see, the Expiration shows up when someone enters the Amount.

So we have two options for the PR:

  1. To make the first PR simple, you can hide Offer Creation fields, such as
  • Destination
  • Amount
  • Expiration
    So, they can be enabled in the next PR.

OR

  1. Add a checkbox Create a Sell offer

That would show those additional fields

  • Destination
  • Amount
  • Expiration

@ihomp
Copy link
Member

ihomp commented May 3, 2025

From a UI perspective:

checkbox: Create an initial Sell offer:
Amount: (Initial listing price in XRP)
Expiration: (How long your offer is valid for)
Destination: (Recipient of NFT, if you use a marketplace as a broker, or Private offers)

The expiration should be hidden when the checkbox Create an initial Sell offer is checked; it should just show Never as default (as it shows now).

  • that can be done in the next PR, for this PR those fields can be hidden.

@ihomp
Copy link
Member

ihomp commented May 3, 2025

It would also be good to split the flags checkboxes from the terms checkboxes. Maybe add some space in between?

@ihomp
Copy link
Member

ihomp commented May 3, 2025

for the fields with addresses, Issuer and Destination for the component, we need to use hideButton={true} to hide the search button.

Also the placeholder texts should be as simple as
Issuer address
Destination address

We need to make sure that when a username is entered, the address is passed to the transaction, not the username.

@pandablue0809
Copy link
Contributor Author

hi @ihomp
I fixed
I added Create a Sell offer in this PR
Plz review again

@ihomp
Copy link
Member

ihomp commented May 10, 2025

I see you use a class mb-2 12 times, I don't see it's defined anywhere. Where does it come from?

sorry, it's my mistake

Just remove them, as they seem unnecessary.
I also found mb-4, mt-4, mt-6

@ihomp
Copy link
Member

ihomp commented May 11, 2025

Component NFTokenMint.js is missing server that should be imported
import { encode, server } from '../../utils'

It is used on the line 420
{server}/nft/{minted}

@ihomp ihomp self-assigned this May 11, 2025
Copy link
Member

@ihomp ihomp left a comment

Choose a reason for hiding this comment

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

requires some changes, as specified in comments

@ihomp
Copy link
Member

ihomp commented May 11, 2025

Mutable flag only works on the devnet for now.
so we need to show only when
network === 'devnet'
where network is from utils.js

@ihomp
Copy link
Member

ihomp commented May 11, 2025

Just as a cleanup:

We can remove:

      Memos: [
        {
          Memo: {
            MemoData: encode('NFT Mint')
          }
        }
      ]

from both requests in NFTokenMint and in URITokenMint

@pandablue0809
Copy link
Contributor Author

hi @ihomp
I fixed
plz check

@pandablue0809 pandablue0809 requested a review from ihomp May 12, 2025 10:00
@ihomp
Copy link
Member

ihomp commented May 12, 2025

I've updated paths for components and utils.. otherwise I couldn't test =)

@pandablue0809
Copy link
Contributor Author

hi @ihomp
I fixed

@ihomp
Copy link
Member

ihomp commented May 12, 2025

The code looks good to me now! 👍

@Anna15170221 Please test:

  1. Xahau: issue NFTs with links like
    http://localhost:3000/en/services/nft-mint?uri=ipfs://QmXwX3C1cBiR8EpSyVhJ4Nq9roMKgeBSw2fYR4e466ph5i&digest=4B3C18C1C54FD793D8F4D3C75A2AE06172A68AF5553929E7527888EC6403D228

  2. XRPL Devnet
    NFT with URI modify flag (it should be visible)

  3. XRPL Testnet:
    NFT with an offer that expires.
    Amount: 0 - should be allowed
    Expiration time should be shown correctly in the issued transaction
    The destination should be shown

There is no indication that we are awaiting the NFT crawler before showing that NFT was minted... but that is a separate ticket we can create after merging this.

Copy link
Member

@ihomp ihomp left a comment

Choose a reason for hiding this comment

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

Looks good!

@Anna15170221
Copy link
Contributor

@ihomp
I've tested point 1 - works fine.
Testnet and Devnet are not currently working well (seems the reason is not from our side - can't sign the transactions, tried with different wallets) - will check later.
JFYI, I've minted nft on the mainnet, expiration date is shown in nft history, but not in transaction itself.
https://bithomp.com/en/nft/0008000049EB0C5CBFBD4CD6558FBE6BCD383F47DB2E3B55491CED2005094E85

@ihomp
Copy link
Member

ihomp commented May 12, 2025

@Anna15170221 works on devnet and testnet for me with CrossMark

with xaman doesn't work when there is a estination for a sellOffer - I've sent them a bug report

@ihomp
Copy link
Member

ihomp commented May 12, 2025

@pandablue0809

when user enters the Amount as 0
it should be added to the transaction
Amount: "0"

I don't think this is the case now.

@ihomp
Copy link
Member

ihomp commented May 12, 2025

We should not add to tx:

Flags: 0

If there are no Flags, do not add Flags to the transaction.

@Anna15170221
Copy link
Contributor

@ihomp
tested Devnet with Gem - worked fine now
https://dev.xrplexplorer.com/en/nft/001827108DCD674725CDA3A730E90CC197647CA8B6C396345236D64800298FAD
and Testnet with Gem
https://test.xrplexplorer.com/nft/000800008DCD674725CDA3A730E90CC197647CA8B6C396343894268F0020E8F4

@ihomp
Copy link
Member

ihomp commented May 13, 2025

I'll merge this, but we need fixes for flags:0 and Amount:0

@ihomp ihomp merged commit 4f85dff into Bithomp:main May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants