MostroP2P / mostro-core

common types used by mostro and clients
https://mostro.network
MIT License
6 stars 7 forks source link

First draft of message version 1 #26

Closed grunch closed 10 months ago

arkanoider commented 10 months ago

Hi @grunch ,

emerging from fiat job, nothing bad in the direction of the PR, i like the idea of using enum container for message like:

pub enum Message {
    Order(MessageKind),
    Dispute(MessageKind),
}

We can go in this direction imo, what i'd like to work on is:

Just free thoughts...

For merge I think that's better to move on dev branch on mostro and mostro-core to have a compilable version in the end.

grunch commented 10 months ago
* Avoid too many modifications in mostro using getter function in mostro-core, for example in this snippet avoid the Some in matching actions
if let Ok(m) = message {
                       let message = Message::from_json(&m);
                       if let Ok(msg) = message {
                           if msg.verify() {
                               match msg.get_action() {
                                   Some(Action::NewOrder) => {
                                       order_action(msg, &event, &my_keys, &client, &pool).await?;
                                   }

sure, can you give a code example?

* Another thing looking back is to try to reduce all kind of message regarding orders to use the same struct, avoiding smallorder, will look if it's too much breakin on mostro side.

If we can remove one of the order struct and unify it in only one would be great

Just free thoughts...

For merge I think that's better to move on dev branch on mostro and mostro-core to have a compilable version in the end.

I think this is a good idea, can you do it?

arkanoider commented 10 months ago
* Avoid too many modifications in mostro using getter function in mostro-core, for example in this snippet avoid the Some in matching actions
if let Ok(m) = message {
                       let message = Message::from_json(&m);
                       if let Ok(msg) = message {
                           if msg.verify() {
                               match msg.get_action() {
                                   Some(Action::NewOrder) => {
                                       order_action(msg, &event, &my_keys, &client, &pool).await?;
                                   }

sure, can you give a code example?

I will push something ASAP

* Another thing looking back is to try to reduce all kind of message regarding orders to use the same struct, avoiding smallorder, will look if it's too much breakin on mostro side.

If we can remove one of the order struct and unify it in only one would be great

I will try to see if it's a big effort or not, could be a nice and cleaner improvement.

Just free thoughts... For merge I think that's better to move on dev branch on mostro and mostro-core to have a compilable version in the end.

I think this is a good idea, can you do it?

Yep we can have branch dev-protocol-version-1 on both mostro and mostro-core? What do you say? We will eventually merge some milestones which are compiling.

grunch commented 10 months ago

I was thinking you were talking about having a dev branch, if you want to have just a branch for message version 1 why we just don't work on this branch? it's easier

arkanoider commented 10 months ago

I was thinking you were talking about having a dev branch, if you want to have just a branch for message version 1 why we just don't work on this branch? it's easier

Yep! That's exactly what I meant...let's go on with this here. I was only saying dev as generic name for another branch message-version-1 is ok!

arkanoider commented 10 months ago

Hi @grunch ,

Removed version from Messagekind, I think it better to have in lib.rs two const globally used:

pub const PROTOCOL_MAJOR_VER: u8 = 1; pub const PROTOCOL_MINOR_VER: u8 = 0; So simply every new message is created like that:

    pub fn new(
        id: Option<Uuid>,
        pubkey: Option<String>,
        action: Action,
        content: Option<Content>,
    ) -> Self {
        Self {
            version_major: PROTOCOL_MAJOR_VER,
            version_minor: PROTOCOL_MINOR_VER,
            id,
            pubkey,
            action,
            content,
        }
    }

We can remove all version parameter from Mostro too, cleaner code imo.

Added also CantDo message as we said yesterday, in Mostro we use a lot, so a quick creator is nice for me.

Will clean Mostro from version parameter meanwhile.

arkanoider commented 10 months ago

Removed