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

test: AztecRPC API using sandbox #1568

Merged
merged 15 commits into from
Aug 17, 2023
Merged

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Aug 15, 2023

  1. Fixes #1504
  2. propagates actual error thrown on the server to the JsonRpcClient (instead of showing the generic Bad Request one).

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@benesjan benesjan marked this pull request as draft August 15, 2023 13:41
@benesjan benesjan force-pushed the janb/testing-aztec-rpc-using-sandbox branch 3 times, most recently from 55ca067 to 28142c8 Compare August 16, 2023 10:41
@benesjan benesjan marked this pull request as ready for review August 16, 2023 12:10
@benesjan benesjan enabled auto-merge (squash) August 16, 2023 14:23
Copy link
Collaborator

@spalladino spalladino left a 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.

Comment on lines 41 to 46
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;
}
Copy link
Collaborator

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:

  1. Include another optional param that takes an error and returns a bool on whether to abort retries or not
  2. Have the function fn receive a parameter that can be used to bail, similar to how async-retry does it
  3. Have fn wrap its non-retriable exceptions in a specific NoRetryException
  4. Or have fn attach a retry: false property to the exception to avoid retries

Copy link
Contributor Author

@benesjan benesjan Aug 17, 2023

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.

@benesjan benesjan force-pushed the janb/testing-aztec-rpc-using-sandbox branch from 09bbe62 to 0b4a3d1 Compare August 17, 2023 06:37
@benesjan benesjan merged commit b2662db into master Aug 17, 2023
1 check passed
@benesjan benesjan deleted the janb/testing-aztec-rpc-using-sandbox branch August 17, 2023 12:29
codygunton pushed a commit that referenced this pull request Jan 23, 2024
* 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 <philip.windle@gmail.com>

* 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 <spypsy@users.noreply.github.com>

* 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 <spypsy@users.noreply.github.com>
Co-authored-by: Suyash Bagad <suyash@aztecprotocol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Test all AztecRPC api endpoints using sandbox
2 participants