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

Outside imports #24

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Outside imports #24

wants to merge 3 commits into from

Conversation

mwufi
Copy link

@mwufi mwufi commented Oct 20, 2021

Motivation

This allows NFT storefront code to be imported outside of this repo!

The contracts package is intentionally small - rather than relying on emulator (like the test package here), we only make it so that it can load up the Cadence files. Then, the emulator will decide what to do with them.

Test plan

Test loading of contracts
make test in the lib/go/contracts directory
image

Existing tests (in lib/go/test)
make test in the lib/go/test directory

@mwufi
Copy link
Author

mwufi commented Oct 20, 2021

@psiemens :) This will be a small step for Jan's comment from before! If we do this, we can potentially have more complicated account setup steps in the emulator (after deploying the contract!)

Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @mwufi! I've left some comments and suggestions

lib/go/contracts/contracts_test.go Outdated Show resolved Hide resolved
}

// ReadWithAddresses loads a .cdc file with its addresses
func ReadWithAddresses(filename string, addressMap map[string]string) []byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func ReadWithAddresses(filename string, addressMap map[string]string) []byte {
func readWithAddresses(filename string, addressMap map[string]string) []byte {

I don't think this function needs to be exported

Comment on lines +14 to +20
SetupAccountTransaction = "transactions/setup_account.cdc"
SellItemTransaction = "transactions/sell_item.cdc"
BuyItemTransaction = "transactions/buy_item.cdc"
RemoveItemTransaction = "transactions/remove_item.cdc"
CleanupItemTransaction = "transactions/cleanup_item.cdc"
GetIDsScript = "scripts/read_storefront_ids.cdc"
GetListingDetailsScript = "scripts/read_listing_details.cdc"
Copy link
Contributor

Choose a reason for hiding this comment

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

These variables don't need to be exported from the package

Copy link
Author

Choose a reason for hiding this comment

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

oh @psiemens it's actually interesting here -- so my intention was to export these names, so that you can load up the scripts later with ReadWithAddresses! Then you can do something similar to what's currently done in the test package (set up accounts, etc).

What's the idiomatic Flow way of doing that?

Comment on lines +34 to +36
if !recognizedAddresses[importFile] {
fmt.Printf("Did you mispell anything? Replacing '%s'...\n", importFile)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of the recognizedAddresses map? Does it just check that. addressMap doesn't contain an address that it doesn't recognize?

IMO you could simplify the code by removing this check -- this function is only called internally and I think it would actually be safer to assign "FungibleToken" and "NonFungibleToken" to constants that the top of this file to avoid spelling mistakes.

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.

2 participants