El Collectooorr review 18/07/22 e7f5eae0c430444ac43aa70ea64c63a5f219bca7

[Adamantios]

This review was performed for commit: e7f5eae

• 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.,:

  agent-academy-1/packages/valory/skills/fractionalize_deployment_abci/behaviours.py

  Line 101 in 3a7412b

  0.15 * (10 ** 18)

  &&

  agent-academy-1/packages/valory/skills/fractionalize_deployment_abci/behaviours.py

  Line 243 in 3a7412b

  safe_tx_gas=10 ** 7,

  &&

  agent-academy-1/packages/valory/skills/elcollectooorr_abci/behaviours.py

  Line 319 in 3a7412b

  if minter == "0x":

  &&

  agent-academy-1/packages/valory/skills/elcollectooorr_abci/behaviours.py

  Lines 465 to 469 in 3a7412b

  	f"The safe contract balance is {safe_balance / 10 ** 18}Ξ."
  	)
  	self.context.logger.info(f"Already spent {already_spent / 10 ** 18}Ξ.")
  	self.context.logger.info(
  	f"The current budget is {current_budget / 10 ** 18}Ξ."

• Exception handling in many behaviours, coming from the use of enforce e.g.,:

  agent-academy-1/packages/valory/skills/fractionalize_deployment_abci/behaviours.py

  Line 125 in 3a7412b

  except AEAEnforceError as e:

  &&

  agent-academy-1/packages/valory/skills/fractionalize_deployment_abci/behaviours.py

  Line 216 in 3a7412b

  except AEAEnforceError as e:

  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:

  agent-academy-1/packages/valory/skills/fractionalize_deployment_abci/rounds.py

  Lines 65 to 72 in 3a7412b

  	def encode_float(value: float) -> bytes:
  	"""Encode a float value."""
  	return struct.pack("d", value)
  	def rotate_list(my_list: list, positions: int) -> List[str]:
  	"""Rotate a list n positions."""
  	return my_list[positions:] + my_list[:positions]

• 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:

  agent-academy-1/packages/valory/skills/elcollectooorr_abci/rounds.py

  Lines 755 to 772 in 3a7412b

  	def _merge_paid_users(old: Dict[str, int], new: Dict[str, int]) -> Dict[str, int]:
  	merged = {}
  	for address, amount in new.items():
  	if address in old.keys():
  	merged[address] = old[address] + amount
  	else:
  	merged[address] = amount
  	return merged
  	def end_block(self) -> Optional[Tuple[BasePeriodState, Event]]:
  	"""Process the end of the block."""
  	already_paid = cast(Dict[str, int], self.period_state.db.get("paid_users", {}))
  	newly_paid = cast(
  	Dict[str, int], self.period_state.db.get("users_being_paid", {})
  	)
  	all_paid_users = self._merge_paid_users(already_paid, newly_paid)

[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 fix/audit problems #90 for all the listed constants here.
• I agree it doesn't make sense to use enforce(), I have created an issue for it Replace AEAEnforceError with a more appropriate mechanism #91 , and it will be worked on it in a future version.
• Those unused functions are removed in fix/audit problems #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?