Skip to content

refactor GPA: typed messages#756

Draft
dessaya wants to merge 5 commits intoiotaledger:developfrom
dessaya:refactor-gpa
Draft

refactor GPA: typed messages#756
dessaya wants to merge 5 commits intoiotaledger:developfrom
dessaya:refactor-gpa

Conversation

@dessaya
Copy link
Contributor

@dessaya dessaya commented Oct 31, 2025

No description provided.

// > Set NeedConsensus <- NIL
if cmi.latestActiveCommittee != nil {
msgs.AddAll(cmi.suspendCommittee(cmi.latestActiveCommittee))
msgs = slices.Concat(msgs, cmi.suspendCommittee(cmi.latestActiveCommittee))
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you feel about these inline msgs.AddAll(doSometing), slices.Concat(msgs, stateMgr.Input(...)) etc?
Would you agree, that this style is very common in this code and that it decreases readbility? How about changing it to something like this:

res := cmi.suspendCommittee(cmi.latestActiveCommittee)
outmsgs = slices.Concat(msgs, resOutMsgs)

Because I personally loose focus when reading code like: msgs.AddAll(stateMgr.Input(NewInputThingyDone(inputArg))) :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I don't find it so distracting, but I'm also not against splitting those lines if it improves readability! Let's do it.

}
msg.SetSender(cni.pubKeyAsNodeID(recv.SenderPubKey))
cni.sendMessages(cni.chainMgr.Message(msg))
cni.sendMessages(cni.chainMgr.Message(gpa.NewMessageIn(cni.pubKeyAsNodeID(recv.SenderPubKey), msg)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would split that into 3-4 lines :D

type BasicMessage struct {
sender NodeID
recipient NodeID
type TypedMessageIn[T MessagePayload] struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you like MessageInT more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like either 😝

func (msg *BasicMessage) SetSender(sender NodeID) {
msg.sender = sender
type (
MessageIn = TypedMessageIn[MessagePayload]
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I do see benefit of what you did here, I also see, that TypedMessageIn[SomePayload] is not automatically convertiable to MessageIn. When it was interface it was convertible.
Didn't this aspect create you any problem while refacroting? Because if not - maybe not is not a real drawback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is why I added the AsTypedMessageIn function, e.g.:

func (cmi *ChainMgr) Message(msg *gpa.MessageIn) []*gpa.MessageOut {
	switch msg.Payload.(type) {
	case *msgCommitteeLog:
		return cmi.handleMsgCommitteeLog(gpa.AsTypedMessageIn[*msgCommitteeLog](msg))
	case *msgBlockProduced:
		return cmi.handleMsgBlockProduced(gpa.AsTypedMessageIn[*msgBlockProduced](msg))
	}
	panic(fmt.Errorf("unexpected message %T: %+v", msg, msg))
}

I'm not too happy with the ergonomics. If only Go had better type inference :(

@dessaya dessaya force-pushed the refactor-gpa branch 4 times, most recently from c08084e to 619c630 Compare November 11, 2025 16:10
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.

2 participants