Skip to content
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

Changes per Git issue 1487 #1510

Merged
merged 6 commits into from
Sep 12, 2022
Merged

Changes per Git issue 1487 #1510

merged 6 commits into from
Sep 12, 2022

Conversation

DennisDawson
Copy link
Contributor

No description provided.

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Link check report. 420542 links checked.
1 broken links found:
File: out/account_nfts.html Link: nftoken.html#taxon

Preview: https://XRPLF.github.io/xrpl-dev-portal/pr-preview/Issue_1487/

Style Report

@@ -49,7 +48,6 @@ This transaction assumes that the issuer, `rNCFjv8Ek5oDrNiMJ3pw6eLLFtMjZLJnf2`,
| Field | JSON Type | [Internal Type][] | Description |
|:--------------|:--------------------|:------------------|:-------------------|
| `NFTokenTaxon` | Number | UInt32 | The taxon associated with the token. The taxon is generally a value chosen by the minter of the token. A given taxon can be used for multiple tokens. Taxon identifiers greater than `0xFFFF'FFFF` are disallowed. |
| `Issuer` | String | AccountID | _(Optional)_ The issuer of the token, if the sender of the account is issuing it on behalf of another account. This field must be omitted if the account sending the transaction is the issuer of the `NFToken`. If provided, the issuer's [AccountRoot object][] must have the `NFTokenMinter` field set to the sender of this transaction (this transaction's `Account` field). |
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this field was necessary when acting as a minter.

Copy link
Contributor Author

@DennisDawson DennisDawson Sep 8, 2022

Choose a reason for hiding this comment

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

Yes but per the Git issue, it's a complex example to start with, given that there's an example of an authorized minter later in the topic. Ach - but I have to remove the paragraph above. Gotcha.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get removing it from the example above but this block is what lists all possible fields. Without this block there is no full definition of the field.

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Link check report. 420542 links checked.
1 broken links found:
File: out/account_nfts.html Link: nftoken.html#taxon

Preview: https://XRPLF.github.io/xrpl-dev-portal/pr-preview/Issue_1487/

Style Report

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Link check report. 420542 links checked.
1 broken links found:
File: out/account_nfts.html Link: nftoken.html#taxon

Preview: https://XRPLF.github.io/xrpl-dev-portal/pr-preview/Issue_1487/

Style Report

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Link check report. 420542 links checked.
1 broken links found:
File: out/ja/account_nfts.html Link: nftoken.html#nftokentaxon

Preview: https://XRPLF.github.io/xrpl-dev-portal/pr-preview/Issue_1487/

Style Report

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

Link check report. 420544 links checked.
1 broken links found:
File: out/ja/account_nfts.html Link: nftoken.html#nftokentaxon

Preview: https://XRPLF.github.io/xrpl-dev-portal/pr-preview/Issue_1487/

Style Report

Copy link
Collaborator

@mDuo13 mDuo13 left a comment

Choose a reason for hiding this comment

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

There are a couple changes I'd like to see in there (several of them being things that slipped through before this PR) but assuming those are adequately addressed, this is a 👍 from me.



### Example

This value sets the transfer fee to 314 bps, or 3.14%.
This value sets the transfer fee to 314, or .00314%.

![Txr Fee](img/nftokenb.png "Txr Fee")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed, this might be the only place the docs use "txr" to abbreviate "transfer".

Suggested change
![Txr Fee](img/nftokenb.png "Txr Fee")
![Transfer Fee](img/nftokenb.png "Transfer Fee")


An issuer might issue several NFTs with the same taxon; to ensure that NFTs are spread across multiple pages, the taxon is scrambled using the fifth section, a dumb sequential number, as the seed for a random number generator. The scrambled value is stored with the `NFToken`, but the unscrambled value is the actual taxon.
An issuer might issue several `NFToken` objects with the same `NFTokenTaxon`; to ensure that `NFToken` objects are spread across multiple pages, the `NFTokenTaxon` is scrambled using the fifth section, a dumb sequential number, as the seed for a random number generator. The scrambled value is stored with the `NFToken`, but the unscrambled value is the actual NFTokenTaxon.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest striking the word "dumb" from this sentence, since it's not necessary and can be an emotionally-charged word. Also, I noticed an inconsistent use of code font for NFTokenTaxon here.

Suggested change
An issuer might issue several `NFToken` objects with the same `NFTokenTaxon`; to ensure that `NFToken` objects are spread across multiple pages, the `NFTokenTaxon` is scrambled using the fifth section, a dumb sequential number, as the seed for a random number generator. The scrambled value is stored with the `NFToken`, but the unscrambled value is the actual NFTokenTaxon.
An issuer might issue several `NFToken` objects with the same `NFTokenTaxon`; to ensure that `NFToken` objects are spread across multiple pages, the `NFTokenTaxon` is scrambled using the fifth section, a sequential number, as the seed for a random number generator. The scrambled value is stored with the `NFToken`, but the unscrambled value is the actual `NFTokenTaxon`.

Comment on lines 202 to 209
<!-- tr>
<td><code>lsfAuthorized</code>
</td>
<td><code>0x00000002</code>
</td>
<td>If set, the offer has been approved by the issuer. This flag can only be set by the <code>Issuer</code> of the token or an account authorized by the issuer (for example, the <code>MintAccount</code> listed in the account root of the <code>Issuer</code>) and only if the token has the flag indicating that authorization is required.
</td>
</tr>
</tr -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than commenting out this item, I suggest deleting it. The text is still there in the Git history if we need to go back and dig it up for some reason.

@@ -11,7 +11,7 @@ status: not_enabled
# NFTokenPage
{% include '_snippets/nfts-disclaimer.md' %}

The `NFTokenPage` object represents a collection of `NFToken` objects owned by the same account. An account can have multiple `NFTokenPage` ledger objects, which form a doubly-linked list (DLL).
The `NFTokenPage` object represents a collection of `NFToken` objects owned by the same account. An account can have multiple `NFTokenPage` ledger objects, which form a doubly linked list (DLL).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the abbreviation "DLL" isn't used again (and .dll has a different meaning in Windows files, so people could get led astray thinking about those), I suggest removing it.

Suggested change
The `NFTokenPage` object represents a collection of `NFToken` objects owned by the same account. An account can have multiple `NFTokenPage` ledger objects, which form a doubly linked list (DLL).
The `NFTokenPage` object represents a collection of `NFToken` objects owned by the same account. An account can have multiple `NFTokenPage` ledger objects, which form a doubly linked list.

@@ -3,7 +3,7 @@ html: nftokenacceptoffer.html
parent: transaction-types.html
blurb: Accept an offer to buy or sell an NFToken.
labels:
- NFTs, Non-fungible Tokens
- NFTokens, Non-fungible Tokens
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you change this label it won't work as a link anymore, so I suggest reverting this instance

@@ -28,7 +28,7 @@ The mode in which the transaction operates depends on the presence of the `NFTok

If neither of those fields is specified, the transaction is malformed and produces a `tem` class error.

The semantics of brokered mode are slightly different than one in direct mode: The account executing the transaction functions as a broker, bringing the two offers together and causing them to be matched, but does not acquire ownership of the involved NFT, which will, if the transaction is successful, be transferred directly from the seller to the buyer.
The semantics of brokered mode are slightly different than one in direct mode: The account executing the transaction functions as a broker, bringing the two offers together and causing them to be matched, but does not acquire ownership of the involved NFToken, which will, if the transaction is successful, be transferred directly from the seller to the buyer.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code font for consistency.

Suggested change
The semantics of brokered mode are slightly different than one in direct mode: The account executing the transaction functions as a broker, bringing the two offers together and causing them to be matched, but does not acquire ownership of the involved NFToken, which will, if the transaction is successful, be transferred directly from the seller to the buyer.
The semantics of brokered mode are slightly different than one in direct mode: The account executing the transaction functions as a broker, bringing the two offers together and causing them to be matched, but does not acquire ownership of the involved `NFToken`, which will, if the transaction is successful, be transferred directly from the seller to the buyer.

@github-actions
Copy link

Link check report. 420544 links checked.
1 broken links found:
File: out/ja/account_nfts.html Link: nftoken.html#nftokentaxon

Preview: https://XRPLF.github.io/xrpl-dev-portal/pr-preview/Issue_1487/

Style Report

@DennisDawson DennisDawson merged commit 5cfd209 into master Sep 12, 2022
@DennisDawson DennisDawson deleted the Issue_1487 branch September 12, 2022 18:26
@mDuo13 mDuo13 mentioned this pull request Sep 15, 2022
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