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

beautify TuxedoGenesisConfig #128

Merged
merged 5 commits into from
Nov 12, 2023
Merged

beautify TuxedoGenesisConfig #128

merged 5 commits into from
Nov 12, 2023

Conversation

muraca
Copy link
Collaborator

@muraca muraca commented Oct 16, 2023

resolves #23
New syntax is:

let genesis_transactions = vec![
// Money Transactions
Coin::<0>::mint(100, SigCheck::new(SHAWN_PUB_KEY_BYTES)),
Coin::<0>::mint(100, ThresholdMultiSignature::new(1, signatories)),
// Kitty Transactions
KittyData::mint(Parent::mom(), b"mother", UpForGrabs),
KittyData::mint(Parent::dad(), b"father", UpForGrabs),
];

This is done by leveraging functions written ad-hoc.

/// Create a mint transaction for a single Coin.
pub fn mint<V, OV, OC>(amt: u128, v: V) -> Transaction<OV, OC>
where
V: Verifier,
OV: Verifier + From<V>,
OC: tuxedo_core::ConstraintChecker<OV> + From<MoneyConstraintChecker<ID>>,
{
Transaction {
inputs: vec![],
peeks: vec![],
outputs: vec![(Self::new(amt), v).into()],
checker: MoneyConstraintChecker::Mint.into(),
}
}

TuxedoGenesisConfig has been moved to tuxedo-core/src/genesis.rs, while the runtime type RuntimeGenesisConfig, the tests and the function that replaces the Default trait implementation have been moved to tuxedo-template-runtime/src/genesis.rs.

tuxedo-template-runtime/src/lib.rs Outdated Show resolved Hide resolved
@JoshOrndorff
Copy link
Contributor

I approved this earlier because it is certainly a big improvement over what we have previously.

But with the genesis extrinsics shaping up in #123 and #127, I wonder if we should forbid runtime developers from manually creating outputs directly, and rather require them to only create genesis extrinsics, which will specify the genesis utxos as their outputs.

I think that requiring genesis transactions encourages the genesis block authors to communicate some semantics to the posterity of their chain. And it also makes the genesis block read more like a creation myth.

@muraca
Copy link
Collaborator Author

muraca commented Oct 17, 2023

I wonder if we should forbid runtime developers from manually creating outputs directly, and rather require them to only create genesis extrinsics, which will specify the genesis utxos as their outputs.

@JoshOrndorff I might agree on the fact that we should put Transactions in the GenesisBuilder, rather than Outputs, as we currently do.
However, I think it's a good idea to use this syntax simplify the process for who wants to bootstrap a chain, since said Transactions should not consume any input or use peeks.

@muraca muraca marked this pull request as draft October 17, 2023 08:45
@JoshOrndorff
Copy link
Contributor

Agreed

@muraca muraca mentioned this pull request Oct 25, 2023
Signed-off-by: muraca <mmuraca247@gmail.com>
@muraca muraca marked this pull request as ready for review November 7, 2023 11:19
@muraca muraca changed the title beautify GenesisConfig beautify TuxedoGenesisConfig Nov 7, 2023
@JoshOrndorff
Copy link
Contributor

Perhaps this is a good time to address some technical debt that Andrew and I introduced early on in order to keep prototyping quickly.

Ideally, the genesis extrinsics for a given chain would be specified in the chain spec. If you look at the Substrate node template or other FRAME chains, they do this. All the genesis state is specified directly in the chain spec. So if I want to launch a node template with my own initial balances or staking authorities, I only edit the chain spec, no need to re-compile anything. This is what we want in Tuxedo too (except it will be genesis transactions, not genesis state.)

The current design where we hard-code it all in the impl Default for GenesisBuilder is the hack / tech debt. Really the default should be something simple like only storing the wasm runtime, and not having any initial funds.

Regarding the piece-specific helper function, I totally support that idea. Those functions could be called form the chain spec.

…doGenesisConfig::default` implementation

Signed-off-by: muraca <mmuraca247@gmail.com>
node/src/chain_spec.rs Outdated Show resolved Hide resolved
Signed-off-by: muraca <mmuraca247@gmail.com>
Signed-off-by: muraca <mmuraca247@gmail.com>
node/src/chain_spec.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

I'm pretty happy with the design here. There are just a couple last things that still seems a little out-of-place to me.

One is the tests in the runtime. Are they just creating some random genesis config and then testing that it was put in the storage correctly? Do they have much to do with the runtime itself?

I guess at minimum they should be moved to a tests.rs file in the runtime, but I wonder if they can just be deleted entirely. Now that the genesis config is better expressed.

Second is the function development_genesis_config(). I'm very happy that this is no longer in the impl Default block. It is already a huge improvement. But I wonder if moving it all the way to the node is too much. What if we put it in the runtime in a dedicated genesis.rs file. I can imagine that in the future we may want to add other similar functions. For example on that takes list of endowed_addresses or something. I followed this pattern of having a genesis module in the runtime back in the Substrate recipes days, and it worked well. Here are Five runtimes that all follow the pattern to see what I mean. https://github.com/JoshOrndorff/recipes/tree/master/runtimes

@muraca
Copy link
Collaborator Author

muraca commented Nov 11, 2023

But I wonder if moving it all the way to the node is too much.

I defaulted to that as it is what the substrate template does, with closures.

What if we put it in the runtime in a dedicated genesis.rs file.

I see the value of this. The genesis should be tied to the runtime in my opinion as well.

I guess at minimum they should be moved to a tests.rs file in the runtime, but I wonder if they can just be deleted entirely. Now that the genesis config is better expressed.

I see the value of testing the genesis config works as expected, and this function is actually testing the mint functions implemented in this PR as well.
I think the right place for these tests is in the runtime/src/genesis.rs along the aforementioned functions.

Signed-off-by: muraca <mmuraca247@gmail.com>
Copy link

Coverage after merging beautify-genesis-config into main will be

64.03%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
tuxedo-core/aggregator/src
   lib.rs99.29%100%100%99.26%117, 21
tuxedo-core/no_bound/src
   clone_no_bound.rs36.63%100%30%37.36%24, 32–39, 56, 59–72, 74–87, 89, 91–95, 98–99
   debug_no_bound.rs32.73%100%30%33%101–105, 108–109, 24, 32–42, 58, 60–78, 80–95, 97–99
   default_no_bound.rs19.11%100%16.67%19.31%100–122, 124–131, 133, 136–147, 151–157, 32–39, 51, 54–80, 83–88, 91–99
   lib.rs100%100%100%100%
tuxedo-core/src
   constraint_checker.rs54.65%100%52.63%55.22%110, 71–91
   dynamic_typing.rs84.03%100%72.73%88.37%57
   executive.rs91.52%100%92.68%91.38%116, 142, 175, 207, 220–228, 236, 247, 298, 325, 376–381, 383, 393–394, 399–400, 403, 405–406, 409–410, 412, 416–437, 439, 443–444, 446–447, 449, 452–469, 54
   genesis.rs45%100%42.86%45.16%100, 152–154, 163, 36–49, 60, 62–65, 67–68, 70–72, 75–79, 81–83, 85–97
   inherents.rs7.41%100%12.50%6.52%120–122, 158–159, 164–178, 180–203, 212–217, 219–228, 230, 56–58, 63–68, 70–78, 81, 83
   types.rs70.21%100%54.35%75.35%13, 143, 36, 86, 88–91, 93–99
   utxo_set.rs90.91%100%100%88.89%39–40
   verifier.rs87.16%100%71.60%91.05%120, 154, 28, 54–55, 68
tuxedo-template-runtime/src
   genesis.rs84.62%100%88.89%84.35%23–45
   lib.rs5.25%100%15.38%3.16%100, 123, 126, 131–133, 137–139, 152, 154, 156, 158, 160, 162, 164, 166, 169, 171, 176–177, 185–206, 209–236, 239–350, 59–64, 95–99
wallet/src
   amoeba.rs0%100%0%0%100–106, 109–110, 112–118, 120–127, 19–48, 51–52, 54–99
   cli.rs0%100%0%0%103, 108, 119, 125, 14, 17, 19, 28, 33, 38, 45, 56, 68, 91
   keystore.rs0%100%0%0%31–33, 38–45, 47–48, 51, 53–59, 65–73, 76–78, 80–82, 85–91, 93–94
   main.rs0%100%0%0%101–102, 105–117, 120, 122–126, 129–133, 135–144, 146–147, 151–161, 164–165, 167, 170–171, 173, 175, 177–180, 182–184, 186, 190–197, 200–203, 205–207, 210–212, 214, 216, 219–225, 228–241, 244–247, 249–259, 26, 260, 262, 27–30, 33, 36–38, 40–41, 44, 46, 48–49, 53, 56–62, 65, 67–69, 73–76, 78, 80, 82–83, 86–87, 89, 91–93, 98–99
   money.rs0%100%0%0%100–101, 105, 109–112, 114, 119–125, 127–131, 134–135, 139–144, 146–147, 22–28, 31–49, 53–59, 65–72, 74, 78–83, 87, 90, 92, 95–98
   output_filter.rs100%100%100%100%
   rpc.rs0%100%0%0%16–21, 24–26, 28–44, 47–53, 55–58, 60–61
   sync.rs0%100%0%0%103–106, 113–114, 116, 119–122, 127–129, 132–133, 136–141, 143–145, 148, 150–151, 156–159, 162–163, 170–174, 176–182, 185–187, 189–190, 193–194, 199–202, 205, 207–208, 213–216, 219, 221–222, 225–231, 233–234, 237–238, 241–242, 245–246, 250–256, 259–263, 266–268, 271–279, 281, 285, 287–288, 291–292, 295–302, 304–305, 308–309, 311, 313–314, 318–320, 322–323, 325–326, 328–329, 332–334, 336–337, 339–340, 342–343, 347, 349–350, 354, 356–361, 364–365, 368–370, 373, 376–379, 381, 384–387, 390, 393–394, 397–398, 403–408, 410, 412, 417–420, 423–424, 427–432, 434, 437–438, 442–443, 445, 447–449, 451–454, 457–458, 46–50, 54, 57–58, 61, 63–77, 82–86, 88–89, 94–99
wardrobe/amoeba/src
   lib.rs58.17%100%26.83%69.64%121, 139–140, 178–179, 81–82
   tests.rs100%100%100%100%
wardrobe/kitties/src
   lib.rs48.56%100%28.93%56.61%100, 102–103, 107–111, 117–119, 121–123, 127–129, 133–134, 137–139, 143–146, 150–152, 154–155, 159–163, 172–192, 211–212, 215–222, 225, 228, 230, 232, 234, 236, 238, 240, 242, 244, 246, 248, 250, 252, 254, 256, 258, 260, 262, 391, 40–42, 423, 426, 44–48, 486, 49–53, 56–58, 60–62, 66–70, 77–79, 81–83, 87–91, 98–99
   tests.rs99.61%100%98.25%99.78%
wardrobe/money/src
   lib.rs49.39%100%22.45%60.87%102–103, 106–113, 116, 119, 121, 123, 127, 130, 133, 191, 22–24, 33–35, 37–38, 41–47, 51, 60–62, 64–66, 69–72
   tests.rs100%100%100%100%
wardrobe/poe/src
   lib.rs0%100%0%0%100, 108–117, 121–122, 128–129, 134–143, 147–151, 153–154, 167–168, 173–179, 39, 52, 55, 57, 59, 61, 65, 84–86,

@muraca muraca merged commit d6c52b8 into main Nov 12, 2023
6 checks passed
@muraca muraca deleted the beautify-genesis-config branch November 12, 2023 09:29
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.

Genesis config
2 participants