Remove watch_mode from NRE

[ericnordelo]

Fixes #308

Check this comment for context.

[andrew-fleming]

Looks good to me! We should create another issue for adding watch_mode to the NRE

[martriay]

I don't think this patches the issue, since the script in the original issue uses Account directly. although we could remove it from there too, I think it's better to properly handle the error. we've been experiencing some instability in the codebase and I think patches/hotfixes like this will only make this worse.

i suggest raising an exception after unregistration on tx failure, and let users handle that exception from the script. what do you think? some better version of:

if watch_mode is not None:
    tx_status = await status(tx_hash, network, watch_mode)
    if tx_status.status.is_rejected:
        deployments.unregister(address, network, alias, abi=register_abi)
        raise Exception("could not deploy")

[ericnordelo]

I don't think this patches the issue, since the script in the original issue uses Account directly. although we could remove it from there too, I think it's better to properly handle the error. we've been experiencing some instability in the codebase and I think patches/hotfixes like this will only make this worse.

The script tested with watch_mode first, and the second issue is a combination of watch_mode and the not accomplished invariant. For me, the script without watch_mode is working, not sure if I missed something but I think it patches the issue. Your proposed solution seems good, but we need to refactor the integration in more commands, and think about the design. I don't think this is a tiny update, and I'm already working on a refactor to better handle transactions (important for adding queries to declare, and deploy_account). I can work on this, but I rather wait till I finish what I'm working on if you agree (switching context is a costly task).

EDIT: IMO we need to take the time to think of a strong design (extensible and maintainable) for integrating exceptions for NRE, not only into status, but the rest of the commands.

[martriay]

this doesn't patch the issue because users can still pass watch_mode to Account, it's available in scripts but not part of NRE's interface, which you patched. that's what Eric does in his script.

agree on solid design, we're in strong debt. but picking between hotfixes, I lean towards a not-thoroughly-designed exception that removing the feature for good -- waiting for txs is very useful when scripting.

[ericnordelo]

this doesn't patch the issue because users can still pass watch_mode to Account, it's available in scripts but not part of NRE's interface, which you patched. that's what Eric does in his script.

Ohh, true, I didn't think it through.

agree on solid design, we're in strong debt. but picking between hotfixes, I lean towards a not-thoroughly-designed exception that removing the feature for good -- waiting for txs is very useful when scripting.

Ok, I will check this issue first before continuing with what I'm working on.

[ericnordelo]

i suggest raising an exception after unregistration on tx failure, and let users handle that exception from the script. what do you think? some better version of:

I was checking the code trying to implement a fairly quick fix from this suggestion, but we can't just raise an exception from the places we currently check status because this flow is also used for CLI (I would need to implement a pattern for exception handling in CLI to allow this, which is not a small fix, and is also not the best design to raise an exception from NRE IMO).

I can't think of a good approach to fix this that is small enough with the current design :(.

although we could remove it from there too, I think it's better to properly handle the error. we've been experiencing some instability in the codebase and I think patches/hotfixes like this will only make this worse.

I agree that hot fixes like this will make this worse, but I don't think we can properly handle the issue without following a proper design (not a small fix). IMO, any quick fix we can come up with is going to be one of these patches/hotfixes you mention. I'd suggest handling this properly even if is not part of this fix-release (and for now either remove the watch_mode from NRE, or ignore it until is properly fixed).

Any suggestion on how to handle this properly in a quick fix is welcome of course.