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

Refactor ValidationContext and ExecutionContext #325

Open
Tracked by #554
plafer opened this issue Jan 5, 2023 · 3 comments
Open
Tracked by #554

Refactor ValidationContext and ExecutionContext #325

plafer opened this issue Jan 5, 2023 · 3 comments
Assignees
Labels
A: breaking Admin: breaking change that may impact operators

Comments

@plafer
Copy link
Contributor

plafer commented Jan 5, 2023

Comes out of a comment by @Farhad-Shabani and ensuing sync discussion. We still need to figure out the details, but a main point of confusion was that validate() was in the Context trait; it's not clear from a first look that all the methods in the Context are used in the validate() function. We can also think of different ways to separate the methods into a few different traits if that helps with readability, and doesn't duplicate methods.

Blocked on #221.

Below is the original comment:

Originally posted by @Farhad-Shabani in #221 (comment)

Is it right that we have a tailored validation and execution function for each of the messages like MsgCreateClient, MsgUpdateClient, etc.?
Couldn’t we use the following pattern (as a general concept) and implement them on each message type?

impl validator<Ctx: RouterContext> {
	pub fn validate(&self, ctx: &mut Ctx) -> Result<(), Error>;
}

impl executor<Ctx: RouterContext> {
	pub fn execute(&self, ctx: &mut Ctx) -> Result<(), Error>;
}

// consider it for MsgCreateClient 
impl validator<MockContext> for MsgCreateClient {...}
impl executor<MockContext> for MsgCreateClient {...}

It looks more modular, maintainable, and extensible if it works! (please direct me to the places if I am missing something. tnx)

@plafer
Copy link
Contributor Author

plafer commented Feb 15, 2023

Suggested grouping of methods: #386 (comment)

@plafer
Copy link
Contributor Author

plafer commented Feb 15, 2023

We should also keep in mind/revisit the host-based API that was discussed in ADR 5.

@Farhad-Shabani Farhad-Shabani removed the A: blocked Admin: blocked by another (internal/external) issue or PR label Feb 22, 2023
@Farhad-Shabani Farhad-Shabani self-assigned this Feb 24, 2023
@Farhad-Shabani Farhad-Shabani added the A: breaking Admin: breaking change that may impact operators label Feb 24, 2023
@Farhad-Shabani Farhad-Shabani modified the milestones: v0.30.0, v0.31.0, v0.32.0 Feb 27, 2023
@Farhad-Shabani Farhad-Shabani modified the milestones: v0.32.0, v0.33.0 Mar 13, 2023
@adizere
Copy link
Member

adizere commented May 27, 2024

We could consider not including this in v1, to avoid ballooning the scope. Let's keep v1 more lean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: breaking Admin: breaking change that may impact operators
Projects
None yet
Development

No branches or pull requests

3 participants