Add configurable Mech request price parameter and migrate mech contract functionality #146
Conversation
…_configurable_request_price
…tract_functionality
jmoreira-valory
changed the base branch from
feat/add_configurable_request_price
to
main
jmoreira-valory
changed the title
| @@ -310,6 +327,7 @@ def async_act(self) -> Generator: | |||
| ) | |||
There was a problem hiding this comment.
This logging should not be present or at least not at an info level. Moreover, serialized_data is a generator.
There was a problem hiding this comment.
Please make a concrete suggestion how to fix this.
There was a problem hiding this comment.
Logging a generator is not useful, there's no content to print if it's not consumed.
The suggestion is to remove this unnecessary log and many more that have been added for debugging purposes to the mech interact skill.
packages/valory/skills/mech_interact_abci/behaviours/response.py
Outdated
Show resolved
Hide resolved
| None, | ||
| None, | ||
| GNOSIS_CHAIN_ID, | ||
| None, | ||
| None, |
There was a problem hiding this comment.
This won't trigger the none_event.
There was a problem hiding this comment.
This was the case in the previous state of the repository (GNOSIS_CHAIN_ID was also present). Please open an issue to fix it in another PR.
There was a problem hiding this comment.
Additionally, no NONE event is specified in the FSM app:
https://github.com/valory-xyz/IEKit/blob/main/packages/valory/skills/mech_interact_abci/rounds.py#L51-L56
There was a problem hiding this comment.
Added issue #155. This has to be addressed on a different PR, as this one is already quite complex.
| @@ -65,6 +65,7 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: | |||
| self.mech_contract_address: str = self._ensure( | |||
| "mech_contract_address", kwargs, str | |||
| ) | |||
| self.request_price: Optional[int] = kwargs.get("request_price", None) | |||
There was a problem hiding this comment.
Why not self._ensure("request_price", ...?
There was a problem hiding this comment.
Maybe this is used in more than one skill and ensure pops.
There was a problem hiding this comment.
Can you point out which skill?
There was a problem hiding this comment.
That was a guess, not an actual assertion.
There was a problem hiding this comment.
It was decided that request_price be an optional parameter. Therefore, cannot use _ensure.
Proposed changes
Adds parameter 'request_price'
Migrate all contract functionality from mech contract on Trader to IEKit (deleted contract in Use mech_interact_abci trader#238). Addressess issue Ensure all mech contract functionality is migrated to this repository #150.
Fixes
If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce? (A breaking change is a fix or feature that would cause existing functionality and APIs to not work as expected.)
Put an
xin the box that appliesChecklist
Put an
xin the boxes that apply.mainbranch (left side). Also you should start your branch off ourmain.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...