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

fix: dynamic chain support #73

Merged
merged 2 commits into from
Mar 11, 2024
Merged

fix: dynamic chain support #73

merged 2 commits into from
Mar 11, 2024

Conversation

chris13524
Copy link
Member

@chris13524 chris13524 commented Mar 8, 2024

Description

Dynamically supports chains that Blockchain API says it supports via this new API. Supported chains are obtained on server startup, and refreshed every 4 hours.

This PR also extracts the Blockchain API functions out of the relay_rpc crate into a new crate because they depend on the tokio dependency which was only a dev-dependency.

This PR also makes the Blockchain API endpoint configurable rather than hardcoded. This is a best practice, but also enables the ability to disable the Blockchain API provider during testing (it's disabled in integration tests) which is good because it's not used anyway and also reduces dependency on Internet services for local development.

How Has This Been Tested?

Manually by running the Notify Server and Keys Server PRs that depend on this locally. Created a custom test case which uses an identity key and signed CACAO extracted from developer tools of running app.web3inbox.com which I created with Argent wallet.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@chris13524 chris13524 self-assigned this Mar 8, 2024
)
.await
{
error!("Failed to refresh supported chains: {e}");
Copy link
Member Author

@chris13524 chris13524 Mar 8, 2024

Choose a reason for hiding this comment

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

Ideally we could get service-level metrics, alarms, etc. for this error, but would take more refactoring and not a big deal anyway because this type of temporary error is non-actionable and we test this code path on app startup.

Address::from_str(&address).map_err(|_| CacaoError::AddressInvalid)?,
&hash.finalize()[..]
.try_into()
.expect("hash length is 32 bytes"),

Choose a reason for hiding this comment

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

why is it ok to panic here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the Keccak256 hash length is always 32 bytes

Copy link

@nopestack nopestack left a comment

Choose a reason for hiding this comment

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

all good, just pointed out a panic that was there before but I'm curious about the rationale behind it.

@chris13524 chris13524 merged commit 593029f into main Mar 11, 2024
10 checks passed
@chris13524 chris13524 deleted the fix/dynamic-chain-support branch March 11, 2024 20:17
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.

3 participants