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

Fireblocks support READ #53

Merged
merged 26 commits into from
Aug 7, 2024
Merged

Fireblocks support READ #53

merged 26 commits into from
Aug 7, 2024

Conversation

supernovahs
Copy link
Collaborator

@supernovahs supernovahs commented Jul 24, 2024

closes #52

  • get_transaction
  • get asset addresses
  • list contracts
  • list vault accounts
  • contract_call
  • fireblocks wallet implementation
  • fireblocks wallet tests

crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/get_transaction.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/get_transaction.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/list_contracts.rs Outdated Show resolved Hide resolved
@supernovahs supernovahs requested a review from dralves July 30, 2024 07:37
@supernovahs
Copy link
Collaborator Author

contract_call pending -> fireblocks/developers-hub#3

think we can merge this till then

crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/client.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/transaction.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/transaction.rs Outdated Show resolved Hide resolved
crates/chainio/clients/fireblocks/src/transaction.rs Outdated Show resolved Hide resolved
@supernovahs
Copy link
Collaborator Author

We can't run the fireblock tests in ci because the api key will expire in 2 weeks . So user will have option to run them locally by adding their keys in env

@supernovahs supernovahs marked this pull request as ready for review August 2, 2024 12:13
@supernovahs supernovahs requested a review from dralves August 2, 2024 12:16
@supernovahs
Copy link
Collaborator Author

writing the readme

@dralves
Copy link
Collaborator

dralves commented Aug 2, 2024

We can't run the fireblock tests in ci because the api key will expire in 2 weeks . So user will have option to run them locally by adding their keys in env

Can you add a ticket to linear so that we can track adding this in CI somehow?

Copy link
Collaborator

@dralves dralves left a comment

Choose a reason for hiding this comment

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

still more comments to come

AUTHORIZATION,
HeaderValue::from_str(&format!("Bearer {}", token))?,
);
headers.insert("X-API-Key", HeaderValue::from_str(&self.api_key)?);
Copy link
Collaborator

Choose a reason for hiding this comment

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

when you don't or can't address the comment, please leave a reason why

Copy link
Collaborator Author

@supernovahs supernovahs left a comment

Choose a reason for hiding this comment

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

^

@supernovahs
Copy link
Collaborator Author

Will add logger using #62 when this gets merged

@supernovahs
Copy link
Collaborator Author

The current tests are tested using sandbox environment using my workspace values. If someone will run them, they would have to modify the test values as of the current state. The only solution to make the tests more general would be to create testing parameters for testnet, which needs a paid fireblocks access .
We can do it in separate PR in future @dralves wdyt? Or should I ask someone for a paid workspace access and modify the test values in this PR only?

@dralves
Copy link
Collaborator

dralves commented Aug 6, 2024

@supernovahs let's do in future PRs

@supernovahs supernovahs requested a review from dralves August 7, 2024 11:14
@supernovahs supernovahs changed the title Fireblocks support Fireblocks support READ Aug 7, 2024
@supernovahs supernovahs merged commit c853f31 into main Aug 7, 2024
1 check passed
@supernovahs supernovahs deleted the supernovahs/fireblocks branch August 14, 2024 18:28
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.

add fireblocks support
4 participants