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

Modify runtime descriptors to support ADR 0004 #3548

Merged
merged 1 commit into from
Jan 13, 2021

Conversation

abukosek
Copy link
Contributor

@abukosek abukosek commented Dec 4, 2020

Closes #3450.

TODO:

  • fix e2e tests.
  • fix unit tests.
  • remove SignedRuntime entirely.
  • double-check that oasis-node debug fix-genesis command does the right thing.
  • write a few changelog fragments.
  • when 3511 is merged, fix the TODO in go/consensus/tendermint/apps/registry/transactions.go.
  • update docs.
  • add enable_runtime_governance_models consensus parameter & tests for it.
  • add more tests.

In a separate PR:

  • add update_runtime runtime message support & e2e tests.

@abukosek abukosek force-pushed the andrej/feature/adr4-part1 branch 12 times, most recently from f49b271 to 6a08f96 Compare December 7, 2020 09:34
@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #3548 (ca84d78) into master (25b63aa) will increase coverage by 0.08%.
The diff coverage is 64.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3548      +/-   ##
==========================================
+ Coverage   67.23%   67.31%   +0.08%     
==========================================
  Files         390      390              
  Lines       36540    36667     +127     
==========================================
+ Hits        24567    24683     +116     
- Misses       8524     8529       +5     
- Partials     3449     3455       +6     
Impacted Files Coverage Δ
go/oasis-node/cmd/ias/auth_genesis.go 47.82% <0.00%> (+1.67%) ⬆️
go/staking/api/slashing.go 77.77% <0.00%> (-9.73%) ⬇️
go/consensus/tendermint/apps/registry/genesis.go 41.74% <36.36%> (+1.18%) ⬆️
go/oasis-node/cmd/registry/runtime/runtime.go 59.13% <41.02%> (+1.44%) ⬆️
go/registry/api/api.go 49.20% <45.23%> (-0.62%) ⬇️
go/registry/api/sanity_check.go 67.17% <50.00%> (+0.68%) ⬆️
...consensus/tendermint/apps/registry/transactions.go 58.69% <59.45%> (+2.07%) ⬆️
go/registry/api/runtime.go 45.85% <61.29%> (+3.20%) ⬆️
.../consensus/tendermint/apps/registry/state/state.go 59.07% <65.00%> (+3.87%) ⬆️
...nsus/tendermint/apps/supplementarysanity/checks.go 41.71% <66.66%> (ø)
... and 35 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 cd82b14...ca84d78. Read the comment docs.

@abukosek abukosek added c:breaking/cfg Category: breaks configuration c:breaking/consensus Category: breaking consensus changes c:breaking/runtime Category: breaking runtime changes labels Dec 7, 2020
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Don't forget to update the documentation and add tests for the new features (whitelist changes, minimum pool size, submitting register runtime txes for consensus/runtime governance types).

go/consensus/tendermint/apps/registry/state/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
go/genesis/genesis_test.go Outdated Show resolved Hide resolved
go/oasis-test-runner/scenario/e2e/registry_cli.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
@abukosek abukosek force-pushed the andrej/feature/adr4-part1 branch 10 times, most recently from 4dc4c56 to 1b50bbd Compare December 17, 2020 13:03
@kostko
Copy link
Member

kostko commented Dec 18, 2020

add update_runtime runtime message support & e2e tests (maybe in a separate PR?).

Yes, this should be a separate PR.

@abukosek abukosek force-pushed the andrej/feature/adr4-part1 branch 2 times, most recently from 3a97740 to f3cb039 Compare December 18, 2020 17:39
@abukosek abukosek force-pushed the andrej/feature/adr4-part1 branch 3 times, most recently from 102c622 to 3f6d9ea Compare January 6, 2021 12:33
docs/consensus/registry.md Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/state/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
go/registry/api/api.go Show resolved Hide resolved
go/registry/api/runtime.go Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
Comment on lines +44 to +45
// XXX: The "0" case is only for backward compatibility, so that the old
// genesis file loads -- remove this once mainnet is upgraded!
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better if we handled this in fixgenesis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because there are too many structures to duplicate and there's more chance of something going wrong with the conversion (and since this is in staking code it's better to be more careful). A two-line fix like this is cleaner, IMO :)
I'll file an issue later, so we don't forget to remove this once the upgrade is complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed as #3627 :)

@abukosek abukosek force-pushed the andrej/feature/adr4-part1 branch 5 times, most recently from aa760c6 to cc754fe Compare January 8, 2021 15:13
@abukosek
Copy link
Contributor Author

abukosek commented Jan 8, 2021

Thanks for the comments! This is now ready for re-review.

@abukosek abukosek marked this pull request as ready for review January 8, 2021 16:00
@abukosek abukosek force-pushed the andrej/feature/adr4-part1 branch 2 times, most recently from 539ef3f to 010f479 Compare January 11, 2021 10:39
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/registry/api/runtime.go Outdated Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
docs/consensus/registry.md Show resolved Hide resolved
go/consensus/tendermint/apps/registry/transactions.go Outdated Show resolved Hide resolved
go/oasis-node/cmd/genesis/genesis.go Outdated Show resolved Hide resolved
@abukosek abukosek force-pushed the andrej/feature/adr4-part1 branch 2 times, most recently from 3f71537 to d51975a Compare January 12, 2021 13:38
@abukosek abukosek merged commit 897b681 into master Jan 13, 2021
@abukosek abukosek deleted the andrej/feature/adr4-part1 branch January 13, 2021 09:47
@abukosek
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:breaking/cfg Category: breaks configuration c:breaking/consensus Category: breaking consensus changes c:breaking/runtime Category: breaking runtime changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ADR 4 (part 1, runtime descriptors)
3 participants