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

feat(sessions): irn client scaffolding #681

Merged
merged 7 commits into from
Jun 25, 2024

Conversation

geekbrother
Copy link
Contributor

@geekbrother geekbrother commented Jun 21, 2024

Description

This PR implements the IRN client abstraction into the blockchain-api. The following changes are made:

CI/CD and build

  • WalletConnect/irn-node repository was added as a git submodule.
  • irn_api and network packages were added as dependencies from the irn-node.
  • BSL-1.0 license was added to the allowed list.

The following IRN client configuration parameters were added:

  • RPC_PROXY_IRN_NODE environment variable and irn_node terraform variable for the IRN node address to connect to in IP:Socket format.
  • RPC_PROXY_IRN_KEY environment variable and irn_key terraform variable for the IRN client private key.
  • RPC_PROXY_IRN_NAMESPACE environment variable and irn_namespace terraform variable for the IRN storage namespace.
  • RPC_PROXY_IRN_NAMESPACE_SECRET environment variable and irn_namespace_secret terraform variable for the IRN storage namespace secret.

The following IRN client storage commands were added:

  • Key-value storage set,get, del.
  • Hashmap storage hset, hget, hdel, hfields, hvalues.

IRN client abstraction is bound to the irn global state to be accessible from handlers.

The record TTL is 30 days. (Which should be ok for sessions).

How Has This Been Tested?

  • Unit test for the helper function was implemented and run in CI.
  • Test to run get, set, del were implemented. The test is ignored by default, because we don't want to spin up a cluster for each CI (and will be covered in the handler integration test).
    • It can be run locally by starting the local cluster and run cargo test -- storage::irn::tests::test_irn_client_set_get_del --exact --show-output --ignored. The test were successfully passed in the local environment.
  • Test to run hashmap operations were implemented. The test is ignored by default, because we don't want to spin up a cluster for each CI (and will be covered in the handler integration test).
    • It can be run locally by starting the local cluster and run cargo test -- storage::irn::tests::test_irn_client_hashmap --exact --show-output --ignored. The test were successfully passed in the local environment.

Due Diligence

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

@geekbrother geekbrother self-assigned this Jun 21, 2024
@geekbrother geekbrother force-pushed the feat/sessions/1-client-scaffolding branch 3 times, most recently from 6c9fea2 to c0479d6 Compare June 21, 2024 21:41
@geekbrother geekbrother force-pushed the feat/sessions/1-client-scaffolding branch from c0479d6 to 0b67934 Compare June 24, 2024 20:13
@geekbrother geekbrother changed the base branch from master to feat/update_ci_version June 24, 2024 20:14
@geekbrother geekbrother force-pushed the feat/sessions/1-client-scaffolding branch from 0b67934 to d6bc453 Compare June 24, 2024 20:17
key_base64.as_bytes(),
irn_api::auth::Encoding::Base64,
)
.map_err(|_| StorageError::WrongCredentialsFormat("wrong key is provided".to_string()))?;
Copy link
Member

@chris13524 chris13524 Jun 25, 2024

Choose a reason for hiding this comment

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

suggestion: Separate the WrongCredentialsFormat variant into 2 variants instead of passing a string to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks! Added in f44679c

.map_err(|_| StorageError::WrongCredentialsFormat("wrong key is provided".to_string()))?;
let peer_id = irn_api::auth::peer_id(&key.verifying_key());
let node_addr = node_addr.parse::<SocketAddr>().map_err(|_| {
StorageError::WrongArgument("node_addr is not in socket address format".to_string())
Copy link
Member

Choose a reason for hiding this comment

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

Similar here: just make a variant for this specific error and pass the original error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks! Added in f44679c

Base automatically changed from feat/update_ci_version to master June 25, 2024 07:17
@geekbrother geekbrother force-pushed the feat/sessions/1-client-scaffolding branch from d6bc453 to b578d0f Compare June 25, 2024 07:18
irn_api::auth::Encoding::Base64,
)
.map_err(|_| StorageError::WrongKey(key_base64))?;
// IRN connection error Network(ConnectionHandler(NoAvailablePeers)) when using
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heilhead I have NoAvailablePeers when generating peer_id from the existing key. It works when generating a new one. Any idea where I am wrong? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on slack, you're trying to use the client peer ID for server address, and this isn't supported by our network machinery. The proper server peer IDs should be parsed as part of the node address, see the following relay code on how to parse them from a {PEER_ID}-{MULTIADDR} string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a follow-up for this #684

@@ -31,3 +31,9 @@ export RPC_PROXY_POSTGRES_URI="postgres://postgres@localhost/postgres"
# export RPC_PROXY_RATE_LIMITING_MAX_TOKENS=100
# export RPC_PROXY_RATE_LIMITING_REFILL_INTERVAL_SEC=1
# export RPC_PROXY_RATE_LIMITING_REFILL_RATE=2

# Uncomment for using the IRN client
# export RPC_PROXY_IRN_NODE=127.0.0.1:3011
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using multiple IRN nodes, as a fall-back mechanism. One node can have a downtime even under normal conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Good follow-up. Created it as #685

@xDarksome
Copy link
Contributor

xDarksome commented Jun 25, 2024

The record TTL is 90 days. (Which should be ok for sessions).

does it need to be 90 days? our current longest living data is 30 days

I'm talking from the perspective of data migrations, if our highest value is going to be 90 days, then next time we decide to do full migration we would need to shadow the data for 90 days to be sure

And it probably will happen in the next 3 month, as we will be transitioning to the permissioned network

@geekbrother
Copy link
Contributor Author

The record TTL is 90 days. (Which should be ok for sessions).

does it need to be 90 days? our current longest living data is 30 days

I'm talking from the perspective of data migrations, if our highest value is going to be 90 days, then next time we decide to do full migration we would need to shadow the data for 90 days to be sure

And it probably will happen in the next 3 month, as we will be transitioning to the permissioned network

It decreased to 30 days. The context: slack.

src/env/mod.rs Outdated
@@ -48,6 +49,7 @@ pub struct Config {
pub profiler: ProfilerConfig,
pub providers: ProvidersConfig,
pub rate_limiting: RateLimitingConfig,
pub irn: IRNConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

https://doc.rust-lang.org/1.0.0/style/style/naming/README.html

In CamelCase, acronyms count as one word: use Uuid rather than UUID. In snake_case, acronyms are lower-cased: is_xid_start.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, Clippy missed this. Don't fully rely on clippy 😅 Fixed in 87fb41f. Thanks!

let result = self.client.get(self.key(key.as_bytes().into())).await;

match result {
Ok(Some(data)) => match String::from_utf8(data) {
Copy link
Contributor

@xDarksome xDarksome Jun 25, 2024

Choose a reason for hiding this comment

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

Why are you returning a String instead of a Vec<u8>? I'm guessing that it's going to be deserialized later anyway

@geekbrother geekbrother merged commit f5626e8 into master Jun 25, 2024
16 checks passed
@geekbrother geekbrother deleted the feat/sessions/1-client-scaffolding branch June 25, 2024 20:32
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.

4 participants