Program config storage #593

JesseAbram · 2024-01-16T22:09:06Z

This adds the storage for program config see.

program_type_definition added to Program Info
programs_data (program_pointer, program_config) added to user's RegisteredInfo

vercel · 2024-01-16T22:09:12Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
entropy-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 18, 2024 4:28pm

ameba23

If i've understood right, the idea here is to add the storage logic for program config in this PR, and the actual evaluating a program together with its config will be done separately once entropyxyz/programs#56 is merged.

Anyway this looks great. Personally i would use Option rather than an empty vector to represent no program config, as in the programs PR evaluate takes an Option<Vec<u8>> for the configuration. And generally i think it makes it more explicit - but this works as it is.

Probably more of an issue for Programs, but it would be nice to have a way to ensure that programs which require a configuration are given a valid configuration at the point of registration / updating, rather than at the point of requesting a signature. I'm not sure if that will be too difficult - as in is there a way that given a ProgramData, we can tell whether the configuration is valid for the program, without actually evaluating it with a message.

I'm not sure if ProgramData is the best name, but i don't have a better proposal.

Generally, i think its going be hard for devs to figure out the difference between auxiliary data and configuration, be nice to have names which make it obvious that auxiliary data can vary for each message whereas configuration only varies for each user.

So thumbs up but leaving this as comment for now as not sure if its still WIP, and changelog needs updating.

JesseAbram · 2024-01-17T13:49:13Z

If i've understood right, the idea here is to add the storage logic for program config in this PR, and the actual evaluating a program together with its config will be done separately once entropyxyz/programs#56 is merged.

yup exactly

Anyway this looks great. Personally i would use Option rather than an empty vector to represent no program config, as in the programs PR evaluate takes an Option<Vec<u8>> for the configuration. And generally i think it makes it more explicit - but this works as it is.

mmm I debated this, I just went for consistency here, Im ok with either but if so then this and pointer should be like this

Probably more of an issue for Programs, but it would be nice to have a way to ensure that programs which require a configuration are given a valid configuration at the point of registration / updating, rather than at the point of requesting a signature. I'm not sure if that will be too difficult - as in is there a way that given a ProgramData, we can tell whether the configuration is valid for the program, without actually evaluating it with a message.

I talked about this with jake, it is on the roadmap but a little more complicated and not getting done soon

I'm not sure if ProgramData is the best name, but i don't have a better proposal.

im sure hernando or jake will come up with something

Generally, i think its going be hard for devs to figure out the difference between auxiliary data and configuration, be nice to have names which make it obvious that auxiliary data can vary for each message whereas configuration only varies for each user.

yeee I mean let's get people using it and then we can tweek, doc etc

So thumbs up but leaving this as comment for now as not sure if its still WIP, and changelog needs updating.

…ram-config-storage

HCastano

The big question for me here is the type_definition in the Programs pallet, and what it's role is

HCastano · 2024-01-18T01:51:49Z

pallets/programs/src/lib.rs

+        /// The type definition of the program
+        pub program_type_definition: Vec<u8>,


What's a type definition in this context? I haven't seen this term used in the issue or in Jake's PR

pretty much it is the serialized json input that the program expects for the config input

this should be called something like "serialized_config" to corospond to the optional config parameter of evaluate of type Option<Vec<u8>>. Programs are not required to have a config, and "type definition" doesn't really make semantic sense

or if this is what describes in JSON what kind of JSON interface a program might take, we should remove this for now as wit-generation from types doesn't exist yet and unlikely going in anytime soon.

I've got a few solutions to check that a JSON config is valid, but the solutions are ugly and they deserve more though first. I suspect there will be a new function on the Program that takes the JSON config as an input string and returns Result<(), Error>.

Im not sure I understand, we talked about this and about how currently a program owner would self set this option so the front end can know and understand what needs to be set as config when registering (or choosing their program)

HCastano · 2024-01-18T01:55:21Z

pallets/relayer/src/lib.rs

@@ -88,12 +88,19 @@ pub mod pallet {
    }
    pub type ProgramPointers<Hash, MaxProgramHashes> = BoundedVec<Hash, MaxProgramHashes>;

+    #[derive(Clone, Encode, Decode, Eq, PartialEqNoBound, RuntimeDebugNoBound, TypeInfo)]
+    #[scale_info(skip_type_params(T))]
+    pub struct ProgramData<T: Config> {


I suggest changing this to ProgramInstance. An instance is made up of code + configuration.

There can be a separation of responsibilities here too. While the Programs pallet manages and tracks bytecode, and the Relayer (or Registry) pallet manages and tracks instances.

ya that works for me

HCastano · 2024-01-18T02:00:40Z

pallets/relayer/src/lib.rs

+    #[scale_info(skip_type_params(T))]
+    pub struct ProgramData<T: Config> {
+        pub program_pointer: T::Hash,
+        pub program_config: Vec<u8>,


While I generally I agree with Peg here in that this should be more explicit as an Option - the Programs pallet already accepts empty programs with just an empty vector, and this kinda matches that API.

Maybe the best people to answer this will actually be on the SDK side. Which of the two options makes their code easier to serialize correctly?

cc @jawndiego @frankiebee ?

yes I also heavily lean towards symmetry on this one as in whatever we decide they both should do it, so no matter what is decided I feel like it should be handled in a follow up PR

HCastano · 2024-01-18T02:02:52Z

pallets/relayer/src/lib.rs

+        /// An account hash changed their program pointers [who, new_programs_data]
+        ProgramPointerChanged(T::AccountId, BoundedVec<ProgramData<T>, T::MaxProgramHashes>),


This'll have to be updated to mention instances, or at least the config side of this too

HCastano · 2024-01-18T02:13:12Z

crates/testing-utils/src/test_client/mod.rs

 ) -> anyhow::Result<<EntropyConfig as Config>::Hash> {
-    let update_program_tx = entropy::tx().programs().set_program(program);
+    let update_program_tx = entropy::tx().programs().set_program(program, program_config);


Yeah so this is defined as type_definition in the Programs pallet but it's a config here. So what is it supposed to be?

…ram-config-storage

JesseAbram added 8 commits January 15, 2024 11:03

add program config storage

142218c

update relayer

addfef3

relayer tests

7dc4b17

fix server tests

78dbbdf

programs benchmark

b894ee1

program benchmarks

252584e

fix all tests

5205337

change function to change program data

813d269

vercel bot deployed to Preview January 16, 2024 22:33 View deployment

JesseAbram requested review from jakehemmerle, HCastano and ameba23 January 17, 2024 01:49

clean

3b4c87a

vercel bot deployed to Preview January 17, 2024 02:19 View deployment

ameba23 reviewed Jan 17, 2024

View reviewed changes

JesseAbram marked this pull request as ready for review January 17, 2024 13:46

add changelog

cb38078

vercel bot deployed to Preview January 17, 2024 14:17 View deployment

ameba23 approved these changes Jan 17, 2024

View reviewed changes

Merge branch 'master' of github.com:entropyxyz/entropy-core into prog…

b7d16ca

…ram-config-storage

vercel bot deployed to Preview January 17, 2024 18:00 View deployment

HCastano reviewed Jan 18, 2024

View reviewed changes

JesseAbram added 2 commits January 18, 2024 10:09

tweaks

e2c08b5

change program data to instance

6b92b1a

JesseAbram requested a review from HCastano January 18, 2024 15:34

Merge branch 'master' of github.com:entropyxyz/entropy-core into prog…

b43e0a1

…ram-config-storage

vercel bot deployed to Preview January 18, 2024 16:28 View deployment

JesseAbram merged commit 9458361 into master Jan 18, 2024
10 checks passed

JesseAbram deleted the program-config-storage branch January 18, 2024 16:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Program config storage #593

Program config storage #593

JesseAbram commented Jan 16, 2024 •

edited

Loading

vercel bot commented Jan 16, 2024 •

edited

Loading

ameba23 left a comment

JesseAbram commented Jan 17, 2024

HCastano left a comment

HCastano Jan 18, 2024

JesseAbram Jan 18, 2024

jakehemmerle Jan 18, 2024

jakehemmerle Jan 18, 2024

JesseAbram Jan 18, 2024

HCastano Jan 18, 2024

JesseAbram Jan 18, 2024

HCastano Jan 18, 2024

JesseAbram Jan 18, 2024

HCastano Jan 18, 2024

HCastano Jan 18, 2024

		/// The type definition of the program
		pub program_type_definition: Vec<u8>,

		/// An account hash changed their program pointers [who, new_programs_data]
		ProgramPointerChanged(T::AccountId, BoundedVec<ProgramData<T>, T::MaxProgramHashes>),

Program config storage #593

Program config storage #593

Conversation

JesseAbram commented Jan 16, 2024 • edited Loading

vercel bot commented Jan 16, 2024 • edited Loading

ameba23 left a comment

Choose a reason for hiding this comment

JesseAbram commented Jan 17, 2024

HCastano left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram commented Jan 16, 2024 •

edited

Loading

vercel bot commented Jan 16, 2024 •

edited

Loading