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

sync/v4 expects to return an empty root instead of 404 #327

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

nemunaire
Copy link
Collaborator

@nemunaire nemunaire commented Oct 17, 2024

In this patch, I tried do fix issues reported on the initial synchronization, when the account is new and no sync happen before.

I was able to test the creation of a new account with the sync/v3, it works as expected, but the changes between v3 and v4 seems to be related to how the function behaves for new accounts: #311 (comment) #326 #270 (comment)

Fixes: ddvk#326

func (app *App) syncGetRootV4(c *gin.Context) {
app.syncGetRoot(c, func(c *gin.Context) {
c.JSON(http.StatusOK, messages.SyncRootV3Response{

Choose a reason for hiding this comment

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

Suggested change
c.JSON(http.StatusOK, messages.SyncRootV3Response{
c.JSON(http.StatusOK, messages.SyncRootV3Request{

Got it to work after changing this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't make sense to respond with a Request. It must be something else.

The difference will be:

{
    "generation": 0,
    "schemaVersion": 0
}

to:

{
    "generation": 0,
    "broadcast": false
}

Perhaps adding the correct schemaVersion as seen in the rest of the code?

@davidpkj davidpkj mentioned this pull request Nov 7, 2024
@ddvk ddvk merged commit 87d79db into ddvk:master Nov 15, 2024
4 checks passed
@SohnyBohny
Copy link

SohnyBohny commented Nov 15, 2024

@ddvk stop! Did you test? For me this PR did not work but needed an additional fix, see #327 (comment)

Should I open a new PR?

@ddvk
Copy link
Owner

ddvk commented Nov 15, 2024

ah, sorry. I'm trying to fix something else (3.15), so I just merged it with the idea of fixing it as well if not ok

ddvk added a commit that referenced this pull request Nov 15, 2024
- fix #327
@ddvk
Copy link
Owner

ddvk commented Nov 15, 2024

@SohnyBohny You can try the master again, should work with empty cloud (tested on 3.15.4.2)

@nemunaire nemunaire deleted the f/new-account-v4 branch November 17, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants