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

Return -1 if clock value not found for batch requests, not undefined #1627

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

vicky-g
Copy link
Contributor

@vicky-g vicky-g commented Jul 7, 2021

Description

What is the purpose of this PR? What is the current behavior? New behavior? Relevant links (e.g. Trello) and/or information pertaining to PR?

If you fetch a wallet with no state on the current cnode to this request, there will not be a response. This fix is to return -1 for those users.

Tests

List the manual tests and repro instructions to verify that this PR works as anticipated. Include log analysis if possible.
❗ If this change impacts clients, make sure that you have tested the clients ❗

Test request:

  const axiosReqParams = {
    baseURL: 'http://cn1_creator-node_1:4004',
    url: '/users/batch_clock_status',
    method: 'post',
    data: {
      walletPublicKeys: [
        // this wallet has data on CN
        '0x4749a62b82983fdcf19ce328ef2a7f7ec8915fe5',

        // these two wallets do not have data on CN
        '0xe5669383629208e8b30dbe9e04e13c325f5cb809',
        '0x13d350973206d8fc2420a11ea4bb9edaa94ef2e1'
      ]
    }
  }

  const userClockValuesResp = (await axios(axiosReqParams)).data.data.users

  console.log(userClockValuesResp)

Current example logs:

[ { walletPublicKey: '0x13d350973206d8fc2420a11ea4bb9edaa94ef2e1',
    clock: 62 } ]

PR fix response from axios:

[ { walletPublicKey: '0x13d350973206d8fc2420a11ea4bb9edaa94ef2e1',
    clock: 62 },
  { walletPublicKey: '0x4749a62b82983fdcf19ce328ef2a7f7ec8915fe5',
    clock: -1 },
  { walletPublicKey: '0xe5669383629208e8b30dbe9e04e13c325f5cb809',
    clock: -1 } ]

How will this change be monitored?

No monitoring necessary

@vicky-g vicky-g added the content-node Content Node (previously known as Creator Node) label Jul 7, 2021
Copy link
Contributor

@hareeshnagaraj hareeshnagaraj left a comment

Choose a reason for hiding this comment

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

LGTM, great find!

@vicky-g vicky-g merged commit 0509340 into master Jul 8, 2021
@vicky-g vicky-g deleted the vg-fix-batch-clock-req branch July 8, 2021 00:31
Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

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

goood find

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content-node Content Node (previously known as Creator Node)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants