Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow switching between mainnet and regtest networks. #1115

Closed
wants to merge 2 commits into from

Conversation

klochowicz
Copy link
Contributor

@klochowicz klochowicz commented Aug 18, 2023

It needs a restart, which is clearly communicated in the app.
Eventually we might have "expert" mode that gets unlocked with a toggle or something, so it can't be accidentally clicked.

Removes the need of deploying two versions and mostly removes the need for
dart_define from the release process.

(I almost finished vergen support that would remove other dart_define, will add it in the morning!). This would unblock us to proper continuous delivery for iOS.

note: I've aligned the distance between the buttons to be consistent (10px) after running the screenshot tool.

Screenshot 2023-08-17 at 10 31 50 pm Screenshot 2023-08-17 at 10 31 42 pm

Removes the need of deploying two versions and mostly removes the need for
dart_define from the release process.
We can abstract away how we retrieve these values, especially that we're doing
    it in multiple places.
Copy link
Contributor

@bonomat bonomat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should merge this as is and I wish we would have had the discussion before putting effort into this.

I'm sorry if these efforts were triggered because of my post on elements.

A few points I can think of from the top of my heads which we need to think through:

  • backup of data: right now it is not safe to remove/uninstall the app because it will delete your data directory. Because of this I would not want to run the regtest build (which needs cleansing once in a while) next to my production environment.
  • incompatible APIs: a continues delivery build might not be always compatible with the latest release. Because of this we potentially need to run a "released"-regtest version next to our continuously deployed version? In this case the feature would make sense but we still would need to release a separate bundle to test against our CD version?
  • incompatible DB migrations: I sometimes install old regtest versions of the app just to see if a bug was present in an older version. For that a full-wipe might be necessary (see point 1). If not, a DB downgrade/migration undo, might not be safe in all cases.

Unfortunately I don't think the time is ready for this feature and we have things which are more important than that.

@klochowicz klochowicz marked this pull request as draft August 18, 2023 21:44
@klochowicz klochowicz added the PoC label Aug 19, 2023
@klochowicz
Copy link
Contributor Author

I'm not sure we should merge this as is and I wish we would have had the discussion before putting effort into this.

That's very reasonable, I should have marked it as a draft, as this was a proof of concept rather than ready for merging idea. For quite some time I believed (now I'm not sure whether that belief was shared) that it would be very hard to switch between the mainnet/testnet versions of the app (earlier called "demo mode" in ItchySats).
When I realised I could apply the change only after restart I realised it might be a solvable problem, so I tried it out.
My intention was to spark a discussion, but perhaps it was not a very good idea without opening at least a GitHub discussion around it.
I've created one now: #1127

I'm sorry if these efforts were triggered because of my post on elements.

It catalysed my thinking into this direction, but ultimately, I'm to blame for going outside of the scope for this iteration, for which I take responsibility. See discussion points for the reasons I tried it out.

A few points I can think of from the top of my heads which we need to think through:

  • backup of data: right now it is not safe to remove/uninstall the app because it will delete your data directory. Because of this I would not want to run the regtest build (which needs cleansing once in a while) next to my production environment.

Off the top of my head, I would say that I would try to restructure our flow to not remove the app / uninstall in these situations - the most obvious way would be to always provide a fresh regtest after switching to mainnet and back (with new seed and db).
Of course, it goes without saying that our backup workflow is an issue and should be dealt with and at the very least we should tackle #1079 as well.

  • incompatible APIs: a continues delivery build might not be always compatible with the latest release. Because of this we potentially need to run a "released"-regtest version next to our continuously deployed version? In this case the feature would make sense but we still would need to release a separate bundle to test against our CD version?

I thought about this one for a while, and realised that we could fully embrace CD and wouldn't need to do releases anymore whilst in the closed beta. (other than for marketing purposes, and done retrospectively, when we feel particular build is good).
I shared these (somewhat controversial) ideas around that in the discussion thread.

  • incompatible DB migrations: I sometimes install old regtest versions of the app just to see if a bug was present in an older version. For that a full-wipe might be necessary (see point 1). If not, a DB downgrade/migration undo, might not be safe in all cases.

for this one, we could consider:

  • testing on different phone than production builds
  • using Simulator
  • using native builds (desktop)

🤔 I think that it can already happen, one of our testers (or us) might try to go to earlier versions themselves and inadvertently break their app.

Ideally, we could at least show a message upon failed migration that states something like
"DB migration failed. Please switch back to build XXXX which worked on this database" to make the recovery / export easier.

Unfortunately I don't think the time is ready for this feature and we have things which are more important than that.

Ultimately it's your call, however please have a read over the reasons this could speed up delivery / experiment workflow / bug fixing and many other aspects of the app. The investment might actually pay off.

@klochowicz
Copy link
Contributor Author

Closing this PR because it doesn't seem like it would get sufficient buy-in.
It can be revived at a later stage if we decide to go into this direction.

@klochowicz klochowicz closed this Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants