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

Emulator Feature: Bootstrap with default contracts #88

Merged
merged 14 commits into from
Dec 8, 2021

Conversation

mwufi
Copy link

@mwufi mwufi commented Sep 29, 2021

Closes: FLIP Fest issue 27 onflow/flip-fest#27

Description

The emulator should be able to deploy contracts when it starts up. We add the following ones:

  • NFT Marketplace
  • NFT
  • FUSD

To get this to work, we do two things:

  1. make the emulator aware of the service account's private key -- this allows us to sign transactions!
  2. Add the default contracts

Notes:

  • For NonFungibleToken, we depend on the flow-nft repo
  • For NFTMarketplace, we add the contract directly -- In the future, we can restructure the nft-marketplace repo so that it's easier to pull from

We also include defaults for sig&hash algo (in start.go). (Does this address #85 ?)

ServiceKeySigAlgo       string        `default:"ECDSA_P256" flag:"service-sig-algo" info:"service account key signature algorithm"`
ServiceKeyHashAlgo      string        `default:"SHA3_256" flag:"service-hash-algo" info:"service account key hash algorithm"`

Tests

GO111MODULE=on go run ./cmd/emulator --contracts
image

GO111MODULE=on go run ./cmd/emulator --contracts --service-priv-key 92ecc6ca2a30b1da782dc6ca6a6da73ad3d9214adb041b5185ca5196bd46e3c7
If you run with a custom private key, it reflects that:
image


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@mwufi
Copy link
Author

mwufi commented Oct 1, 2021

hello @janezpodhostnik @psiemens @sideninja @turbolent :) tagging just in case you didn't see!

Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Looks very good overall. Thank you for this!

I left some comments.

cmd/emulator/start/start.go Outdated Show resolved Hide resolved
server/contracts.go Outdated Show resolved Hide resolved
server/contracts/NFTStorefront.cdc Show resolved Hide resolved
blockchain.go Outdated Show resolved Hide resolved
blockchain.go Outdated Show resolved Hide resolved
server/contracts.go Outdated Show resolved Hide resolved
server/contracts.go Outdated Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/contracts.go Outdated Show resolved Hide resolved
@sideninja
Copy link
Contributor

@mwufi Great effort so far, left some comments. Another general comment is don't forget to update docs with any new flags you add.

@sideninja
Copy link
Contributor

Left a question. Also please make sure the CI tests are passing. By taking a quick look it might be you need to tidy the dependencies.

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2021

Codecov Report

Merging #88 (8a1c173) into master (25e6ae5) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #88   +/-   ##
=======================================
  Coverage   55.35%   55.35%           
=======================================
  Files          20       20           
  Lines        1738     1738           
=======================================
  Hits          962      962           
  Misses        672      672           
  Partials      104      104           
Flag Coverage Δ
unittests 55.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25e6ae5...8a1c173. Read the comment docs.

@mwufi
Copy link
Author

mwufi commented Oct 8, 2021

Update for posterity!

tl;dr - the logic with the keys might be refactored in the future!

For now, it seems OK.

  • Currently, whether you pass a private key, public key, or keep the default key, only the public key is passed to the serverConf! Then, when blockchain starts, this makes it create a new service key that overwrites the default (UH OH!)
  • this is different than in the test cases, since in the tests, we make a new Blockchain and don't call WithServicePublicKey
  • So, the line I added will check if there's a (default or passed) private key before attempting to overwrite it!

i think @sideninja is moderately happy with this...

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

LGTM, great work 🚀

@mwufi
Copy link
Author

mwufi commented Oct 9, 2021

@sideninja i go mod tidy'd the merge conflicts! good to go!

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.

Great work @mwufi!

server/server.go Outdated Show resolved Hide resolved
server/contracts.go Outdated Show resolved Hide resolved
Copy link
Contributor

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

LGTM!

@mwufi
Copy link
Author

mwufi commented Oct 20, 2021

Hey @psiemens can you quickly review onflow/fusd#2? Made a tiny change to merge before adding it to this one!

@sideninja
Copy link
Contributor

Hey @psiemens can you quickly review onflow/fusd#2? Made a tiny change to merge before adding it to this one!

I will take a look at this @mwufi

@sideninja
Copy link
Contributor

Hey @psiemens can you quickly review onflow/fusd#2? Made a tiny change to merge before adding it to this one!

I will take a look at this @mwufi

@mwufi onflow/fusd#2 Approved and merged

@mwufi
Copy link
Author

mwufi commented Oct 21, 2021

Added FUSD!
image

return []byte(code)
}

func deployContract(name string, contract []byte, b *emulator.Blockchain) (flow.Address, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention to have contracts deployed to different accounts, else you can deploy all at once.

Copy link
Author

Choose a reason for hiding this comment

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

ah, interesting!! How do you deploy all at once?

Haha I thought it was all right to have different accounts for each one, but they could also go ito the same account... the bigger thing will probably be.. how do you control the accounts once you've deployed the contracts?

  • actually it might be a good idea to make them all deploy from the service acct, since that's what you have the keys for, right?

Copy link
Contributor

@sideninja sideninja Nov 2, 2021

Choose a reason for hiding this comment

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

Yes, I think that it would be better to deploy on the service account not just because of the keys but also there are for sure a lot of tests/projects, unfortunately, depending on the sequence of addresses used for testing so if you will "use" those addresses to do these deployments we might break something for them. You can deploy them using a single transaction as contracts can be passed as an array, see here: https://pkg.go.dev/github.com/onflow/flow-go-sdk/templates#CreateAccount

So maybe better change this and then we should be all good.

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Looks good

@mwufi
Copy link
Author

mwufi commented Oct 30, 2021

Awesome, thanks @sideninja !!! (other team members, feel free to merge, since i don't have merge permissions!)

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Great improvement. I think this is it 🎉

@mwufi
Copy link
Author

mwufi commented Nov 2, 2021

wait, actually i made a mistake!! with the second way, it didn't actually submit the transactions, just created them :( so now i'm figuring out how to run the transactions !!

alternatively, would it be a good idea to create one (1) new account for the contracts? instead of deploying to the service acct?

image

@sideninja
Copy link
Contributor

wait, actually i made a mistake!! with the second way, it didn't actually submit the transactions, just created them :( so now i'm figuring out how to run the transactions !!

alternatively, would it be a good idea to create one (1) new account for the contracts? instead of deploying to the service acct?

image

I planned to test this tomorrow, so will keep you updated. We would only merge this after the Flip fest so no worries.
I would advise you to deploy on service account since you have the keys and for the reasons explained before, people assume second address is something they have always got as a second address and now this would shift that.

{"ExampleNFT", serviceAcct, "✨ NFT contract"},
{"NFTStorefront", serviceAcct, "✨ NFT contract"},
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Use emulator instance SendTransaction function here.

Copy link
Author

Choose a reason for hiding this comment

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

oh!!! will give that a try -- right now i'm just using the raw blockchain methods :P

image

Copy link
Contributor

@sideninja sideninja Nov 2, 2021

Choose a reason for hiding this comment

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

Yeah you are right sorry, no method like that on the blockchain. Maybe anyhow extract to some helper.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, okay! I think this'll do it (now it actually checks the service account before printing, to make sure!)

image

@sideninja sideninja merged commit 6e68009 into onflow:master Dec 8, 2021
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.

5 participants