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(parsed_transaction_history): add transaction pagination #14

Merged
merged 6 commits into from
May 14, 2024

Conversation

rhh4x0r
Copy link
Contributor

@rhh4x0r rhh4x0r commented May 13, 2024

Added ability paginate the results of the request in the transaction history, by doing the following:

  1. Creating a new request type called ParseTransactionHistoryRequest that has an address String and an Option before String
  2. Updated parsed_transaction_history to input the new type, and update the API call if before exists. Updated documentation above it.
  3. Updated the example to reflect the change

I've never contributed to an open-source project, so please forgive me if this is the wrong process for suggesting edits or updates to the module.

I looked through the code to adhere to similar structures that y'all used in other aspects, but either way this should be a complete fix here or a good start to this implementation.

Any feedback or comments please let me know. Otherwise it should be good to go.

Cheers,

rhh

@0xIchigo 0xIchigo changed the title Feat/add transaction pagination feat(parsed_transaction_history): add transaction pagination May 14, 2024
@0xIchigo 0xIchigo self-requested a review May 14, 2024 04:51
Copy link
Collaborator

@0xIchigo 0xIchigo left a comment

Choose a reason for hiding this comment

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

Wow, thank you for your contribution! That was a fast turnaround, from raising the issue in Discord to making a PR! I've left a few minor comments, but this is a solid PR overall!

Can you also please run the cargo fmt command before you commit your new changes? That way everything is formatted properly and all the checks for the build pass

@@ -31,14 +31,20 @@ impl Helius {
///
/// # Arguments
/// * `address` - An address for which a given parsed transaction history will be retrieved
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need to update this to reflect the new request type as the address argument will no longer be valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

parse_transactions follows this format as well (showing the parameters of the type) which is why I framed it that way. Would you like me to update that one as welll?

Copy link
Contributor Author

@rhh4x0r rhh4x0r May 14, 2024

Choose a reason for hiding this comment

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

As in, I put address and before instead of `ParsedTransactionHistoryRequest', like parse_transactions does above
CleanShot 2024-05-14 at 08 11 14@2x

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, yeah, that's an oversight on my end — parse_transactions should have ParseTransactionsRequest as its argument and the description should stay the same

@@ -210,6 +210,12 @@ pub struct ParseTransactionsRequest {
pub transactions: Vec<String>,
}

#[derive(Serialize, Deserialize, Debug)]
pub struct ParseTransactionHistoryRequest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer if we renamed it to ParsedTransactionHistoryRequest so it follows the same name as its function with request added at the end

Copy link
Collaborator

@0xIchigo 0xIchigo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks, and congrats on your first open-source contribution! 🔥

@0xIchigo 0xIchigo merged commit 7b24e7a into helius-labs:dev May 14, 2024
1 check passed
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.

2 participants