search

onflow / nft-catalog

https://www.flow-nft-catalog.com/
The Unlicense
37 stars 13 forks source link

Fix dapper transaction signers. #77

Closed aishairzay closed 2 years ago

vercel[bot] commented 2 years ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
nft-catalog ✅ Ready (Inspect) Visit Preview Sep 19, 2022 at 4:28PM (UTC)
aishairzay commented 2 years ago

Why change the ordering of dapp, dapper and buyer? Let's it keep the same order as in the original template transactions like this.

I'm okay with either, Albert mentioned we might want these changed, and I may have misunderstood and changed the wrong things. I'll let Albert clarify here, and I'll adjust the PR accordingly.

albeethekid commented 2 years ago

Dapper's backend makes assumptions based on the order of the args. Can we stick to this order?

transaction(merchantAccountAddress: Address, storefrontAddress: Address, listingResourceID: UInt64, expectedPrice: UFix64) {

chandanworkacct commented 2 years ago

Also for the "prepare" statements, Dapper FCL expects a certain order of arguments. Let's stick to this https://github.com/dapperlabs/dapper-supported-transactions/blob/master/purchase-nft-direct/purchase-nft-direct.cdc#L19 .

On Fri, Sep 16, 2022 at 11:15 AM Albert Khasky @.***> wrote:

Dapper's backend makes assumptions based on the order of the args. Can we stick to this order?

transaction(merchantAccountAddress: Address, storefrontAddress: Address, listingResourceID: UInt64, expectedPrice: UFix64) {

— Reply to this email directly, view it on GitHub https://github.com/dapperlabs/nft-catalog/pull/77#issuecomment-1249666045, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIFCOFYO6OJOQ336M5KNWELV6S2LXANCNFSM6AAAAAAQNZX47M . You are receiving this because you commented.Message ID: @.***>

chandanworkacct commented 2 years ago

@aishairzay Can we also add something in the comment section of the generated code:

aishairzay commented 2 years ago

Made a few changes including the suggestions you've mentioned @chandanworkacct , as well as reverting the transaction signer order changes.

@albeethekid In the current dapper marketplace transactions we have, there is no merchantAccountAddress, is the reordering of parameters still needed, or should we be adding this parameter? Do you have links to example transactions we should base off of if so?