-
Notifications
You must be signed in to change notification settings - Fork 295
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
test: AztecRPC
API using sandbox
#1568
Conversation
55ca067
to
28142c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I believe there are two changes needed though.
if (err.cause == 'thrownByServer') { | ||
// If the error is specifically marked as thrown by the server, we don't retry because the error should be | ||
// propagated. This is because it's an "intentional error" (e.g. "Contract is not deployed") and not | ||
// a connection error or an unexpected software bug. | ||
throw err; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this belongs here: retry
is a generic util, used in many different contexts, not just json-rpc client/server. It shouldn't know about this particular cause
. I think the clean version to do it is either:
- Include another optional param that takes an error and returns a bool on whether to abort retries or not
- Have the function
fn
receive a parameter that can be used to bail, similar to how async-retry does it - Have
fn
wrap its non-retriable exceptions in a specificNoRetryException
- Or have
fn
attach aretry: false
property to the exception to avoid retries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a good point. I think that given our specific use case, which is "forward all the errors to the client if the error was actually thrown by the server code", option 1 is too impractical (I want all the server thrown errors to be bubbled up so listing them all is too verbose) and option 2 is in my opinion unnecessarily generic (I don't need to do anything special with different errors, I just want it to not retry on server-thrown errors).
For this reason I think option 3 or 4 are the way to go.
I think option 3 will result in a more readable code so I will go with it.
Thank you for for laying out the options so thoroughly.
Addressed in e1d40d9.
09bbe62
to
0b4a3d1
Compare
* Initial attempt to have Kebab perform deployments (#1558) * Initial attempt to have Kebab perform deployments * Fix e2e tests, don't redeploy if given rollup contract * Use correct verification key * Bug fix * VK fix * Added curl to Falafel docker image * WIP * WIP * Updated Faucet * WIP * Deploy mainnet fork alongside kebab (#1556) * Deploy mainnet fork alongside kebab * Added backend block * Updated kebab TF Co-authored-by: PhilWindle <[email protected]> * WIP * Fixed env var name * WIP * Yarn lock files * Revert explorer and zk-money changes * Use devnet specific private key * Additional logging * Attempt to fix block explorer * Reverted unnecessary change * Fix bigint literals and remove hosted sdk e2e test * Fixes Co-authored-by: spypsy <[email protected]> * Fixed command ordering (#1566) * Fixed scripting (#1567) * More TF fixes (#1571) * Force contract redeploy (dev) (#1568) * force contract redeploy + add port to kebab health check * undo port specification for healthcheck * Log the number of drips provided by the faucet (#1518) * Pw/devnet deployment fixes (#1574) * Terraform changes to restart Falafel and Faucet on redeployment * New Devnet chain id * Fix pkey srs size to (n + 1) while loading a pkey. (#1550) * Pw/devnet deployment fixes (#1577) * Terraform changes to restart Falafel and Faucet on redeployment * New Devnet chain id * Fixed Falafel Dev Terraform * Fixed Faucet Dev Terraform (#1578) * Updated Wasabi Terraform for Dev and Test nets (#1579) * Fix for Wasabi Uniswap Terraform (#1580) * update JSON provider request method (#1588) * update JSON provider request method * comment clarification * allow additional methods to go through kebab auth (#1589) * Allow for setting of Rollup Provider in deployments (#1590) * Allow for setting of Rollup Provider in deployments * Force contract redeployment * Terraform fix * Pw/testnet deployment (#1591) * WIP * WIP * WIP * TF updates * Dev TF fix Co-authored-by: spypsy <[email protected]> Co-authored-by: Suyash Bagad <[email protected]>
Bad Request
one).Checklist:
Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.