AgentRegister for AgentCommunication #24
Conversation
| @@ -46,39 +97,43 @@ contract AgentCommunication is Ownable { | |||
| return (amountForTreasury, amountForAgent); | |||
| } | |||
|
|
|||
| // We don't add `onlyRegisteredAgent` modifier here, because anyone should be able to send a message to an agent | |||
There was a problem hiding this comment.
Everything is guarded by onlyRegisteredAgent, except for this, because I guess we still want to be able to send messages from Streamlit UI to agents
| mapping(address => bool) public registeredAgents; | ||
| address[] private registeredAgentsList; |
There was a problem hiding this comment.
Consider using EnumerableMap (https://docs.openzeppelin.com/contracts/5.x/api/utils#EnumerableMap) instead of mapping + list, more efficient.
| function getAtIndex(uint256 idx) | ||
| public | ||
| view | ||
| onlyRegisteredAgent | ||
| returns (DoubleEndedStructQueue.MessageContainer memory) | ||
| { | ||
| return DoubleEndedStructQueue.at(queues[agentAddress], idx); | ||
| return DoubleEndedStructQueue.at(queues[msg.sender], idx); | ||
| } |
There was a problem hiding this comment.
I would say reading should be allowed, as it's not modifying state. Messares are public.
Popping (on the other hand) should be guarded (as you implemented).
| } | ||
| } | ||
|
|
||
| contract AgentCommunication is AgentRegistry { |
There was a problem hiding this comment.
Any reason for coupling these 2 contracts?
In my head, it would make more sense that AgentCommunication receives an AgentRegistry instance (actually the Interface) via constructor arg, and then operate on that.
This way, we keep the contracts independent of each other, since one handles registry and the other handles messaging. Wdty?
There was a problem hiding this comment.
I tried this approach initially but had some errors with it. I'll try it once more and post them here, maybe you will be able to help. (I like it like that more as well)
There was a problem hiding this comment.
Cool, happy to huddle or help async.
What I had in mind (hopefully works) is something like this
contract AgentRegistry {
[...]
}
contract AgentCommunication is Ownable {
IAgentRegistry public agentRegistry;
constructor(IAgentRegistry agentRegistry) Ownable(msg.sender) {
agentRegistry = agentRegistry
}
// setter - onlyOwner
}
No description provided.