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

[ASI-860] Enforce max wallet size in content-node batch_clock_status #2768

Merged
merged 8 commits into from
Mar 31, 2022

Conversation

theoilie
Copy link
Contributor

@theoilie theoilie commented Mar 28, 2022

Description

  • Enforces Snapback’s MAX_BATCH_CLOCK_STATUS_BATCH_SIZE directly in content-node’s /users/batch_clock_status route

Tests

  • Added test to verify error 400 when batch size is exceeded

How will this change be monitored? Are there sufficient logs?

I'm not aware of any health checks for this endpoint. Consumers of the route would rely on the error message covered in a test: Number of wallets must not exceed ${maxNumWallets} (reduce 'walletPublicKeys' field in request body).

@theoilie theoilie requested review from dmanjunath and SidSethi and removed request for dmanjunath March 28, 2022 15:12
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.

this looks great - nice work!!

one thing i just thought of - it would be nice to set a lower value for local dev to ensure that the batching is properly tested each time, maybe 3 or 4

let me or @dmanjunath know if want to talk through how to do that

also nice to see you added the test!

ps - there's a Monitoring / logging section in the PR description template we should not remove. mind filling that in?

creator-node/test/pollingTracks.test.js Outdated Show resolved Hide resolved
@dmanjunath
Copy link
Contributor

@SidSethi not sure if we should diverge the batch size between local and other envs. might be easiest to override the value locally if you want to change it?

@SidSethi
Copy link
Contributor

@SidSethi not sure if we should diverge the batch size between local and other envs. might be easiest to override the value locally if you want to change it?

i dont feel strongly about this but it seems like it would be pretty valuable to have the batching flow tested each time
can we not set the default to 5000 in config.js and set the local value in compose/env/base.env or compose/env/common.env?

@dmanjunath
Copy link
Contributor

not really sure what having a lower threshold in dev does but i'll trust your call. we can move this to an env var and set non-dev default to 5000 and set a development env var override to 5

@pull-request-size pull-request-size bot added size/M and removed size/S labels Mar 28, 2022
@theoilie theoilie force-pushed the theo-asi860-enforce-max-wallet branch from 45d0261 to 09584ca Compare March 28, 2022 19:48
@theoilie theoilie force-pushed the theo-asi860-enforce-max-wallet branch from 09584ca to 73592c9 Compare March 28, 2022 20:01
@theoilie
Copy link
Contributor Author

thanks for the feedback! I addressed all the comments, so please let me know if there are any other changes you want me to make @SidSethi

@SidSethi
Copy link
Contributor

not really sure what having a lower threshold in dev does but i'll trust your call. we can move this to an env var and set non-dev default to 5000 and set a development env var override to 5

yeah the main benefit would be to have each maddog run actually test this but since maddog base run only creates 2 users we would not get any benefit except for in manual testing at which point we could just manually override as needed

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.

looks great! one thing to change

creator-node/src/snapbackSM/snapbackSM.js Outdated Show resolved Hide resolved
creator-node/src/snapbackSM/snapbackSM.js Outdated Show resolved Hide resolved
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.

looks great!

couple things

  1. lets update the Monitoring/logging section to specify the exact error msg
  2. lets make sure that the base maddog test passes locally still (it usually errors in CI, idk why)

@theoilie
Copy link
Contributor Author

looks great!

couple things

  1. lets update the Monitoring/logging section to specify the exact error msg
  2. lets make sure that the base maddog test passes locally still (it usually errors in CI, idk why)

updated the README and restarted mad dog in Circle (it passed in the 2nd-most-recent merge from master)

@theoilie theoilie merged commit 8324e45 into master Mar 31, 2022
@theoilie theoilie deleted the theo-asi860-enforce-max-wallet branch March 31, 2022 23:17
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 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.

None yet

3 participants