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.

Sign up for GitHub

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

Jump to bottom

Multiple contracts per address support - code migration #144

Merged
merged 26 commits into from
Nov 23, 2020
Merged

Multiple contracts per address support - code migration #144

merged 26 commits into from
Nov 23, 2020

Conversation

janezpodhostnik
Copy link
Contributor

@janezpodhostnik janezpodhostnik commented Nov 11, 2020 •
edited
Loading

ref: onflow/flow#82

Migration for code registers.

error will be thrown in the following cases:

  • cannot parse the deployed code. As this isn't currently legal it shouldn't happen.
  • code is deployed but there is no declaration. Possibly there are just comments deployed. Not sure if this is currently legal @turbolent
  • more than two definitions are deployed. Most likely one contract and multiple interfaces, as that the only legal option right now. The interfaces could depend on each other and we don't know how to add the imports correctly without a lot of work.
  • the two definitions that are deployed are both interfaces or both contracts. This is currently illegal. Should not happen.

I ran this on devnet16. There were no errors, which means all contracts are migratable.

@janezpodhostnik janezpodhostnik self-assigned this Nov 11, 2020
ramtinms
ramtinms reviewed Nov 12, 2020
View reviewed changes
migrations/multiple_contract_migration.go Outdated
m.logger.Debug().
Str("address", address.Hex()).
Msg("Single contract or interface at address moved to new key")
p.Key = addNameToKey(p.Key, program.Declarations[0].DeclarationIdentifier().Identifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to change the migration to not impact the original payloads in the migration steps, right now we return the pointer to the original stored values.

ramtinms
ramtinms reviewed Nov 12, 2020
View reviewed changes
migrations/multiple_contract_migration.go Outdated
newKey := key
newKey.KeyParts = make([]ledger.KeyPart, 3)
copy(newKey.KeyParts, key.KeyParts)
newKeyString := fmt.Sprintf("code.%s", name)
Copy link
Contributor

Choose a reason for hiding this comment

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

the long term plan was "code" be a key part type and we only use the name, so the ledger can optimize the code storage but I'm fine with this migration for now.

janezpodhostnik
janezpodhostnik commented Nov 16, 2020
View reviewed changes
cmd/util/ledger/migrations/multiple_contract_migration.go
}
declarations := program.Declarations

// find import declarations
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@turbolent does the import part look good to you?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good!

turbolent
turbolent reviewed Nov 16, 2020
View reviewed changes
cmd/util/ledger/migrations/multiple_contract_migration.go Outdated
return []ledger.Payload{p}, nil
}

// We have two declarations. Due to the current rules one of them is an interface and one is a contract.
Copy link
Member

Choose a reason for hiding this comment

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

Currently zero or one contract, and zero or more contract interfaces are allowed. Was the idea here that we only support the case where we have a contract and a contract interface, because that's the only case in the real world (on the current testnet and mainnet)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats correct. The migration only supports the cases where there is:

  • no contracts/interfaces
  • one contract/interface (it will never be one interface as that is currently not allowed)
  • one contract and one interface

@janezpodhostnik janezpodhostnik changed the base branch from feature/multiple-contract-support to master November 23, 2020 19:26
@janezpodhostnik janezpodhostnik changed the base branch from master to feature/multiple-contract-support November 23, 2020 19:26
@janezpodhostnik janezpodhostnik marked this pull request as ready for review November 23, 2020 19:58
turbolent
turbolent approved these changes Nov 23, 2020
View reviewed changes
Copy link
Member

@turbolent turbolent 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, great work and great tests!

cmd/util/ledger/migrations/multiple_contract_migration.go Show resolved Hide resolved
cmd/util/ledger/migrations/multiple_contract_migration.go Show resolved Hide resolved
cmd/util/ledger/migrations/multiple_contract_migration.go Show resolved Hide resolved
@turbolent turbolent merged commit 70e7a77 into feature/multiple-contract-support Nov 23, 2020
@turbolent turbolent deleted the janez/multiple-contract-support-code-migration branch November 23, 2020 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbolent turbolent turbolent approved these changes

@ramtinms ramtinms ramtinms approved these changes

@Kay-Zee Kay-Zee Awaiting requested review from Kay-Zee

@m4ksio m4ksio Awaiting requested review from m4ksio

@psiemens psiemens Awaiting requested review from psiemens

Assignees

@janezpodhostnik janezpodhostnik

Labels
None yet
Projects
None yet
Milestone
Sprint 28 - Post Beta Mainnet - Part ...
Development

Successfully merging this pull request may close these issues.
4 participants
@janezpodhostnik @turbolent @ramtinms @timmymmit