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

DEVX-291c #1425

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

DEVX-291c #1425

wants to merge 13 commits into from

Conversation

TheChronicMonster
Copy link
Contributor

No description provided.

chrisnevers
chrisnevers previously approved these changes Sep 16, 2022
examples/simple-nft-minter/index.rsh Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
Copy link
Contributor

@chrisnevers chrisnevers left a comment

Choose a reason for hiding this comment

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

Besides these comments, it looks good to me

examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
Copy link
Contributor

@chrisnevers chrisnevers left a comment

Choose a reason for hiding this comment

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

LGTM after these comments

examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
examples/simple-nft-minter/index.mjs Outdated Show resolved Hide resolved
@chrisnevers
Copy link
Contributor

I think gen.log should be gitignored

Copy link
Contributor

@chrisnevers chrisnevers left a comment

Choose a reason for hiding this comment

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

LGTM, I think Jay needs to sign off though

examples/simple-nft-minter/index.mjs Show resolved Hide resolved
f: null, // freeze address
defaultFrozen: false,
reserve: null,
note: Uint8Array[1],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is just undefined and I'm surprised it is not an error

const transferNFT = async (minter, receiver, nftId, supply) => {
const preAmtNFT = await logBalance(minter, nftId);

if (stdlib.connector == 'ALGO') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should always do it and note that it does nothing on ETH

Comment on lines +25 to +27
const getBal = async (who, tok) => tok ? (await stdlib.balanceOf(who, tok)) : fmt(await stdlib.balanceOf(who));

const logBalance = async (acc, tok) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In these two functions, you have an "acceptable" path on tok being undefined. I think it is kinder to use (who, tok = false), to better communicate to the reader that often you expect it to not be provided

f: null, // freeze address
defaultFrozen: false,
reserve: null,
note: Uint8Array[1],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that you should use the ARC spec and have the note actually contain what it is supposed to


const opts = {
supply: 1,
url: "ipfs://bafybeigdyrzt5...", //asset url
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is more compelling if you demonstrate actually creating it too. The same information that you're going to put in the Note field, you want to upload to IPFS.

This page has a little tutorial:

https://js.ipfs.tech/

under "Adding data to IPFS"

If you are nervous about including it in the code always or having our CI do it, etc, then at least make a little program on the side (or a way to run this program) and comment on it in your guide

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.

None yet

3 participants