fix!: leftovers work for sync fallback #5794
Conversation
|
Thank you for opening this pull request! Looks like you have BREAKING CHANGES in your PR. |
Jenkins BuildsClick to see older builds (50)
|
qfrank
force-pushed
the
feat/enable_sync_fallback
branch
2 times, most recently
from
9c847bb
to
837f682
Compare
|
cc @glitchminer looks like this is a good case for some user stories 🙂 |
There was a problem hiding this comment.
Looks good, though more than half the code is mine 🙈
@qfrank do you want to address Igor's comments or do you want me to do it?
nvm I did it :D
|
I added a commit with the review comment fixes. I also rebased and updated my status-desktop PR here: status-im/status-desktop#15820 It works again as expected |
|
@qfrank @jrainville are you still around this PR? |
qfrank
force-pushed
the
feat/enable_sync_fallback
branch
2 times, most recently
from
8eea23b
to
da30cca
Compare
updated @igor-sirotin |
|
@qfrank feel free to squash my commits if you want |
qfrank
force-pushed
the
feat/enable_sync_fallback
branch
from
1b4176c
to
b501c88
Compare
Needed for status-im/status-desktop#15750 Adds an AC notification when the syncing fails and the user is prompted to use a seed phrase instead. There is one notification for the initiator (created) and one for the old account (received). Once the flow is completed, ie the receiver presses Enable and sync, the notifications are deleted
Parveshdhull
force-pushed
the
feat/enable_sync_fallback
branch
from
b501c88
to
016595b
Compare
|
Hi @qfrank, rebased the PR - status-im/status-mobile#21090 (comment) |
| @@ -1063,8 +1063,8 @@ func (api *PublicAPI) SyncDevices(ctx context.Context, name, picture string) err | |||
| return api.service.messenger.SyncDevices(ctx, name, picture, nil) | |||
| } | |||
|
|
|||
| func (api *PublicAPI) EnableAndSyncInstallation(request *requests.EnableAndSyncInstallation) error { | |||
| return api.service.messenger.EnableAndSyncInstallation(request) | |||
| func (api *PublicAPI) EnableInstallationAndSync(request *requests.EnableInstallationAndSync) (*protocol.MessengerResponse, error) { | |||
There was a problem hiding this comment.
As I understand, this is the only breaking change in this PR.
If you really think we should rename it, then:
- Mark
EnableAndSyncInstallationas deprecated, but keep it working. - Introduce
EnableInstallationAndSyncand update clients to use the new endpoint.
This way we can ensure clients are still compatible with the changes.
There was a problem hiding this comment.
I'm not fully aware of all the implications, but as I understood it, the breaking change it bigger than that. Once this is released, people will only be able to pair between 2.31 and up.
I guess it was not possible to keep backwards compatibility, otherwise Andrea would have thought of it.
As for renaming the API, both desktop and mobile are creating PRs at the same time, so I think it's fine to just rip the band aid right now and not keep an unused API?
There was a problem hiding this comment.
I think it depends on the terminology of a breaking change that we should agree upon.
Let me discuss this with the @status-im/status-go-guild today, I'll be back with an answer in the evening.
There was a problem hiding this comment.
We have 3 types of breaking change:
- Breaking between versions (the type that we want to avoid the most)
- Breaking of functionality (basically introducing a (or several) bug(s))
- Breaking between
status-goand client (potentially this case)
I think breaks between status-go and clients can be managed, as long as they are managed they are only a problem if someone has an old branch and doesn't rebase against the latest develop.
There was a problem hiding this comment.
I have added the old API back, can I get your approval now? 🙂 Your approval is valuable for me! @igor-sirotin
There was a problem hiding this comment.
@qfrank I'm not really saying that we should stick to the old function name.
But there's a way to make this carefully: create a new function and deprecate the old one. Then the switch to the new function name will be seamless 🙌
There was a problem hiding this comment.
create a new function and deprecate the old one
I've done this yesterday 🙂
There was a problem hiding this comment.
Can I merge it now? I'm waiting for your approval 🥹 @igor-sirotin
There was a problem hiding this comment.
Oh! I only read your comment and and didn't check the code 😄
There was a problem hiding this comment.
no worries, we can create another PR if needed
igor-sirotin
changed the title
This is a continue work to PR
2 Major changes in this PR:
relate mobile PR