Expose deployment of common contracts to `Blockchain` type #367

m-Peter · 2023-05-05T14:44:32Z

Work towards: onflow/developer-grants#148

Description

We introduce a new option, called Contracts, in order to programmatically specify which contracts should a Blockchain deploy (this is mostly for the common contracts, it does not affect the system contracts, which are essential to the function of the emulator/blockchain).

The end goal is to be able to utilize it in other libraries, for example the cadence-tools/test repo, with something like:

blockchain = newBlockchain(
    emulator.WithCoverageReportingEnabled(true),
    emulator.Contracts(emulator.CommonContracts),
)

This will enable the Cadence Testing Framework to have the common contracts available by default. e.g: NonFungibleToken / MetadataViews / FUSD / ExampleNFT / NFTStorefront / NFTStorefrontV2 etc.

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

blockchain.go

codecov-commenter · 2023-05-05T14:50:56Z

Codecov Report

Merging #367 (b4ab4b8) into master (1bb2689) will decrease coverage by 0.56%.
The diff coverage is 46.84%.

@@            Coverage Diff             @@
##           master     #367      +/-   ##
==========================================
- Coverage   47.54%   46.98%   -0.56%     
==========================================
  Files          26       26              
  Lines        3418     3418              
==========================================
- Hits         1625     1606      -19     
- Misses       1662     1688      +26     
+ Partials      131      124       -7

Flag	Coverage Δ
unittests	`46.98% <46.84%> (-0.56%)`	⬇️

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

Impacted Files	Coverage Δ
blockchain.go	`68.27% <9.09%> (+0.14%)`	⬆️
contracts.go	`46.73% <46.73%> (ø)`
server/server.go	`45.79% <100.00%> (+2.20%)`	⬆️

bluesign · 2023-05-05T14:53:29Z

I think we need a different injection system for this.

I think blockchain should not know about contracts but should get them as parameter somehow. Ideally we would have fvm bootstrap those. We should have an issue on that.

m-Peter · 2023-05-06T12:31:49Z

Thanks for the comment @bluesign 🙏
My first approach, usually revolves around at keeping the changes contained, that's why I just refactored the existing functionality. If the team behind flow-go is open to such an addition, I could give it a shot.
Another approach, is to just expose publicly the common contract deployment logic (contracts.go file), so that cadence-tools/test package could utilize it.
What do you think about this?

bluesign · 2023-05-06T12:42:30Z

I think first step we can send them via struct at first ( like DeployDescription here ) to blockchain. emulator.WithContracts can took []DeployDescription etc.

Then maybe we can bump the issue: onflow/flow-go#2812 If fvm can expose us contracts in bootstrap, it maybe better than to deploy them afterwards. ( So we will have them all deployed even when block height is zero )

But I think we can merge this if we just change it to []DeployDescription, so people can bootstrap via overflow, flowkit etc ( we can even expose default contracts.go

m-Peter · 2023-05-06T13:00:17Z

Perfect, much thanks for the detailed explanation 🙇
I will make the changes, and ping you 🙏

We expose the DeployContracts method to the public, and allow users to pass in their contracts of choice, for deployment. We also expose the CommonContracts global variable, which pre-configures the most common contracts. We also introduce a new option, called WithContracts, in order to programmatically specify which contracts should be deployed by the Blockchain.

devbugging · 2023-05-12T14:36:51Z

blockchain.go

@@ -158,6 +157,7 @@ type config struct {
 	TransactionValidationEnabled bool
 	ChainID                      flowgo.ChainID
 	CoverageReportingEnabled     bool
+	WithContracts                []ContractDescription


Maybe drop the With prefix, or rename to something like InitialContracts, or StartupContracts, or just Contracts

Makes sense, I wanted it to be on par with the server flag, which has a boolean value. But since we changed that, I forgot to update accordingly. I would go with just Contracts.

Updated in b4ab4b8

devbugging · 2023-05-12T14:37:54Z

blockchain.go

@@ -449,6 +461,12 @@ func NewBlockchain(opts ...Option) (*Blockchain, error) {
 	if err != nil {
 		return nil, err
 	}
+	if conf.WithContracts != nil && len(conf.WithContracts) > 0 {


You can just check for len(), because if nil it would just be 0.

Ah, good suggestion 🙌

Fixed in b4ab4b8

devbugging · 2023-05-12T14:38:33Z

blockchain.go

@@ -943,7 +961,7 @@ func (b *Blockchain) GetTransactionResult(ID sdk.Identifier) (*sdk.TransactionRe
 // GetAccountByIndex returns the account for the given address.
 func (b *Blockchain) GetAccountByIndex(index uint) (*sdk.Account, error) {

-	generator := flow.NewAddressGenerator(sdk.ChainID(b.vmCtx.Chain.ChainID()))
+	generator := sdk.NewAddressGenerator(sdk.ChainID(b.vmCtx.Chain.ChainID()))


why this changes?

Both flow and sdk were actually the same package imported twice, and the editor displayed a linting error. So I kept only one of the imports. Did I misunderstood this one?

devbugging · 2023-05-12T14:39:50Z

contracts.go

+	nftstorefront "github.com/onflow/nft-storefront/lib/go/contracts"
+)
+
+var CommonContracts = func() []ContractDescription {


NIT: I think this could be a variable not a function. But I'm not sure if better since then you could modify it.

I am new to Golang 😅. Could I have a global variable definition, with a body? Because I need the chain and service address intermediate variables.

I'm also kinda new to Go, but I think yes. What you could do is:

var chain = flowgo.Emulator.Chain() var ftAddress = flowsdk.HexToAddress(fvm.FungibleTokenAddress(chain).HexWithPrefix()) var serviceAddress = flowsdk.HexToAddress(chain.ServiceAddress().HexWithPrefix()) var CommonContracts = []ContractDescription{ ... }

But this is NIT, so decide however you feel like.

I personally like the function version, because all the intermediate variables, are not exposed to the rest of the file 😇

devbugging

Left some minor comments, otherwise looks great, thank you!

m-Peter · 2023-05-15T13:52:00Z

cc @turbolent @SupunS

This is the necessary ground-work, for the Deployed Contracts improvement.
It will allow integration tests to directly import the core contracts from the service account address, e.g:

import ArrayUtils from "ArrayUtils.cdc"
import FUSD from 0xf8d6e0586b0a20c7

pub contract StringUtils {
    ...
}

Once this is merged, I will open up a follow-up PR on the cadence-tools/test repository, with the necessary changes. To provide better flexibility, we might also bootstrap the configuration mapping:

blockchain.useConfiguration(Test.Configuration({
    "NonFungibleToken": blockchain.serviceAccount().address,
    ...
}))

Exposing the blockchain.serviceAccount(), will also be quite essential, in order to transfer FLOW tokens to other accounts, from the service account.

devbugging · 2023-05-16T10:07:57Z

@m-Peter good to merge?

m-Peter · 2023-05-16T10:15:15Z

@sideninja I think yes, we can proceed with merging this 🙏

m-Peter requested review from devbugging, turbolent, janezpodhostnik and bluesign as code owners May 5, 2023 14:44

m-Peter commented May 5, 2023

View reviewed changes

blockchain.go Outdated Show resolved Hide resolved

m-Peter force-pushed the blockchain-enable-contract-deployment branch from a96675f to e45a531 Compare May 12, 2023 09:30

devbugging reviewed May 12, 2023

View reviewed changes

devbugging approved these changes May 12, 2023

View reviewed changes

Apply suggestions from PR review

b4ab4b8

m-Peter changed the title ~~Move deployment of common contracts to Blockchain type~~ Expose deployment of common contracts to Blockchain type May 15, 2023

m-Peter requested review from devbugging May 15, 2023 12:16

devbugging approved these changes May 15, 2023

View reviewed changes

devbugging merged commit 3c8ca15 into onflow:master May 16, 2023

m-Peter deleted the blockchain-enable-contract-deployment branch June 5, 2023 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose deployment of common contracts to `Blockchain` type #367

Expose deployment of common contracts to `Blockchain` type #367

m-Peter commented May 5, 2023 •

edited

Loading

codecov-commenter commented May 5, 2023 •

edited

Loading

bluesign commented May 5, 2023

m-Peter commented May 6, 2023

bluesign commented May 6, 2023

m-Peter commented May 6, 2023

devbugging May 12, 2023

m-Peter May 12, 2023

m-Peter May 15, 2023

devbugging May 12, 2023

m-Peter May 12, 2023

m-Peter May 15, 2023

devbugging May 12, 2023

m-Peter May 12, 2023

devbugging May 12, 2023

devbugging May 12, 2023

m-Peter May 12, 2023 •

edited

Loading

devbugging May 12, 2023

devbugging May 12, 2023

m-Peter May 15, 2023

devbugging left a comment

m-Peter commented May 15, 2023

devbugging commented May 16, 2023

m-Peter commented May 16, 2023

Expose deployment of common contracts to Blockchain type #367

Expose deployment of common contracts to Blockchain type #367

Conversation

m-Peter commented May 5, 2023 • edited Loading

Description

codecov-commenter commented May 5, 2023 • edited Loading

Codecov Report

bluesign commented May 5, 2023

m-Peter commented May 6, 2023

bluesign commented May 6, 2023

m-Peter commented May 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-Peter May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devbugging left a comment

Choose a reason for hiding this comment

m-Peter commented May 15, 2023

devbugging commented May 16, 2023

m-Peter commented May 16, 2023

Expose deployment of common contracts to `Blockchain` type #367

Expose deployment of common contracts to `Blockchain` type #367

m-Peter commented May 5, 2023 •

edited

Loading

codecov-commenter commented May 5, 2023 •

edited

Loading

m-Peter May 12, 2023 •

edited

Loading