El Collectooorr review 18/07/22 `e7f5eae0c430444ac43aa70ea64c63a5f219bca7`

[Adamantios]

This review was performed for commit: e7f5eae0c430444ac43aa70ea64c63a5f219bca7

• Not using contract_method_call in contracts. -> I also noticed that our contract_method_call only accepts kwargs. It does not accept args. Maybe this is why it was not used?
• Many contract calls have code duplication. e.g., get_multiple_projects_info and get_multiple_projects_minter.
• We need some constants, e.g.,: https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/fractionalize_deployment_abci/behaviours.py#L101 && https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/fractionalize_deployment_abci/behaviours.py#L243 && https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/elcollectooorr_abci/behaviours.py#L319 && https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/elcollectooorr_abci/behaviours.py#L465-L469
• Exception handling in many behaviours, coming from the use of enforce e.g.,: https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/fractionalize_deployment_abci/behaviours.py#L125 && https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/fractionalize_deployment_abci/behaviours.py#L216 Rationale: enforce should be used in lower levels, not inside behaviours. Also, exception handling in behaviours looks dirty and might mean that we can create methods at lower level modules to avoid that. Resolution: In many examples of the ElCollectooorr's code, the enforce is used at the response's contents, instead of checking the state of the contract call's response. If we check that the response is successful instead, we do not need to check for the contents.
• Unused methods: https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/fractionalize_deployment_abci/rounds.py#L65-L72
• I'd suggest the decision models get wrapped into a Task, because in the future we would probably want to add more computationally intensive models.
• Processing needs to happen on the behaviour side: https://github.com/valory-xyz/agent-academy-1/blob/3a7412b3bc10e4bf8ef6616df43771d25bc827f7/packages/valory/skills/elcollectooorr_abci/rounds.py#L755-L772

[0xArdi]

• I wasn't aware of contract_method_call, is there any benefit to using it in comparison to what we have now?
• I have introduced constants in #90 for all the listed constants here.
• I agree it doesn't make sense to use enforce(), I have created an issue for it #91 , and it will be worked on it in a future version.
• Those unused functions are removed in #90 .
• For more complicated algorithms, we will need to use Tasks.
• The reason why this merging is done on the round is to avoid sending large payloads. Apart from it being kinda ugly, do you see any other issues?

[Adamantios]

I wasn't aware of contract_method_call, is there any benefit to using it in comparison to what we have now?

No there is no additional benefit apart from uniformity across repos.

The reason why this merging is done on the round is to avoid sending large payloads. Apart from it being kinda ugly, do you see any other issues?

I see, sending large payloads is not a good idea. One reason to keep all the processing on the behaviour side would be for example to separate the concerns or to enhance readability, i.e., it is easier to read a behaviour and understand the skill. @DavidMinarsch would you have anything to add on this?