-
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
Initial version of helper.ts for CW721-base #131
Conversation
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.
Nice work here.
Added a lot of very minor comments on it, just some tips to polish it up to a very high quality interface.
Hope this is working well for you in your UI.
contracts/cw721-base/helpers.ts
Outdated
}, | ||
} | ||
|
||
const hackatomOptions: Options = { |
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.
These two are pointing to the same network with different faucet, right? Why not unify them?
contracts/cw721-base/helpers.ts
Outdated
// tokenInfo: () => Promise<any> | ||
minter: () => Promise<any> | ||
numTokens: () => Promise<any> | ||
tokens: (owner:string, start_after: string, limit: number ) => Promise<TokensResponse> |
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.
start_after and limit are optional here as well
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.
Actually, probably nicest to use startAfter
for the interface and convert in the json sent over the wire.
contracts/cw721-base/helpers.ts
Outdated
allAllowances: (owner: string, startAfter?: string, limit?: number) => Promise<AllAllowancesResponse> | ||
allAccounts: (startAfter?: string, limit?: number) => Promise<readonly string[]> | ||
contractInfo: () => Promise<any> | ||
ownerOf: (owner: string) => Promise<any> |
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 should be tokenId: string
not owner: string
contracts/cw721-base/helpers.ts
Outdated
allTokens: (start_after?: string, limit?: number ) => Promise<TokensResponse> | ||
|
||
// actions | ||
mint: (token_id: string, owner:string, name:string, description?: string) => Promise<string> |
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.
Also takes image?: string
I believe
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 think defining type TokenId = string
somewhere above and using it some places would make the return values clearer - no runtime or type-check difference, just more readable. Actually, what is the string returned here... I guess not the TokenId.
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.
Oh, and for public API, nice to use tokenId
rather than token_id
contracts/cw721-base/helpers.ts
Outdated
}; | ||
|
||
const allAllowances = async (owner: string, startAfter?: string, limit?: number): Promise<AllAllowancesResponse> => { | ||
return client.queryContractSmart(contractAddress, {all_allowances: { owner, start_after: startAfter, limit }}); |
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.
👍
contracts/cw721-base/helpers.ts
Outdated
}; | ||
*/ | ||
// mints tokens, returns ? | ||
const mint = async (token_id: string, owner: string, name:string, description:string): Promise<string> => { |
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 defined as description?: string
in the interface above. good to be consistent. (and nice to add image url here as well)
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.
Nice cleanup!
@ethanfrey, from my point of view helper.ts is ready to merge. Let me know if there is something what I should improve. |
Sorry for the delay reviewing this. I have been on holiday. I will check this now. But only one of the 2 github committers has signed the CLA. Could you fix this? |
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 quite good. Some small changes requested.
Mainly just replacing any with concrete types.
contracts/cw721-base/helpers.ts
Outdated
return wallet; | ||
}; | ||
|
||
const buildFeeTable = (feeToken: string, gasPrice: number): CosmWasmFeeTable => { |
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.
You can remove buildFeeTable
now, it is unneeded and handles inside the CosmWasmSigningClient
contracts/cw721-base/helpers.ts
Outdated
return accounts.accounts; | ||
}; | ||
|
||
const minter = async (): Promise<any> => { |
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 would be good to clarify the return types here. (And all the any
until ownerOf
).
You can either just return the response verbatum, like allowance
, or pull out the important field like allAccounts
contracts/cw721-base/helpers.ts
Outdated
} | ||
|
||
// total number of tokens issued | ||
const numTokens = async (): Promise<any> => { |
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.
again, is this a number? a stringified number? (from Uint128)? please define type
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.
Also, I would put all queries together and all messages/transactions together, just for readability
@ethanfrey , I guess we can close this ticket. My collegues has been fixed the issues, so that this pull request is outdated. What do you think? |
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.
Sorry for the delay I missed the notification that this was fixed and it slipped.
Merging now,
This is my initial version of helper.ts for CW721-base contract.
npx @cosmjs/cli@^0.23 --init contracts/cosmons/helpers.ts
Following methods are implemented and will be part of helper.ts - comments below will be part of next version. Here for review:
Setup Contract
Transfer Token to Partner
mine.transferNft(partnerAddr, "monster112a9lf95atqvyejqe22xnna8x4mfqd75tkq2kvwcjyysarcsb");
Approve Contract
// Don't call transferNft before, otherwise it fails
mine.approve(address, "monster112a9lf95atqvyejqe22xnna8x4mfqd75tkq2kvwcjyysarcsb");
Queries