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.

Sign up for GitHub

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

Jump to bottom

Add getEventsByCreationNumber #37

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Add getEventsByCreationNumber #37

merged 1 commit into from
Oct 13, 2023

Conversation

0xmigo
Copy link
Contributor

@0xmigo 0xmigo commented Oct 13, 2023 •
edited
Loading

Description

  1. Add Indexer query - getEventsByCreationNumber
  2. e2e

Test Plan

pnpm fmt

Related Links

@0xmigo Graphite App
Copy link
Contributor Author

0xmigo commented Oct 13, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@0xmigo 0xmigo mentioned this pull request Oct 13, 2023
Merged
@0xmigo 0xmigo requested a review from a team October 13, 2023 05:29
@0xmigo 0xmigo marked this pull request as ready for review October 13, 2023 05:29
heliuchuan
heliuchuan reviewed Oct 13, 2023
View reviewed changes
src/api/event.ts Show resolved Hide resolved
src/internal/event.ts Show resolved Hide resolved
src/internal/event.ts Outdated
const { aptosConfig, address, creationNumber } = args;
const graphqlQuery = {
query: GetEventsByCreationNumber,
variables: { address, creationNumber },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
variables: { address, creationNumber },
variables: { Hex.fromHexInput({ hexInput: address }).toString(), creationNumber },

maybe add some input validation? oof I hate the object as params for this method....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

src/internal/queries/getEventsByCreationNumber.graphql Outdated
@@ -0,0 +1,12 @@
query getEventsByCreationNumber($address: String, $creationNumber: bigint) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you are going to do getEventsByEventHandle and getEvents, so prob want to switch this to passing in a where condition - same comment @0xmaayan made in #15 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That make sense!

0xmaayan
0xmaayan reviewed Oct 13, 2023
View reviewed changes
src/internal/event.ts Outdated Show resolved Hide resolved
gregnazario
gregnazario reviewed Oct 13, 2023
View reviewed changes
tests/e2e/api/event.test.ts Outdated
Comment on lines 7 to 11
test("it should get fund events by creation number and address", async () => {
const config = new AptosConfig({ network: Network.TESTNET });
const aptos = new Aptos(config);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to be consistent here and use LOCAL

@0xmigo 0xmigo force-pushed the jin/getEventsByCreationNumber_indexer_query branch from 40ccc2e to adab170 Compare October 13, 2023 17:27
@0xmigo
Copy link
Contributor Author

0xmigo commented Oct 13, 2023

Update:

  1. Switch from TESTNET to LOCAL
  2. Fixed grammars error
  3. Switch to use where clause condition

@0xmigo 0xmigo force-pushed the jin/getEventsByCreationNumber_indexer_query branch from adab170 to 2a73e81 Compare October 13, 2023 17:53
@0xmigo
Copy link
Contributor Author

0xmigo commented Oct 13, 2023

Update 2

Renamed from getEventsByCreationNumber to getEvents for flexibility of future events queries.

gregnazario
gregnazario reviewed Oct 13, 2023
View reviewed changes
Copy link
Collaborator

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Can we change the upstream branch to main and get this in?

Have we tested it out and it works?

@0xmigo
Copy link
Contributor Author

0xmigo commented Oct 13, 2023

Update 2

Renamed from getEventsByCreationNumber to getEvents for flexibility of future events queries.

@gregnazario Yep the whole stack is e2e tested and it works. Is there any concern if I merge this in as a stack? (Including the previous PR in the stack)

gregnazario
gregnazario approved these changes Oct 13, 2023
View reviewed changes
src/internal/event.ts
Comment on lines +26 to +51
export async function getEventsByCreationNumber(args: {
aptosConfig: AptosConfig;
address: HexInput;
creationNumber: AnyNumber;
}): Promise<GetEventsResponse> {
const { aptosConfig, creationNumber } = args;
const address = AccountAddress.fromHexInput({ input: args.address }).toString();

const whereCondition: any = {
account_address: { _eq: address },
creation_number: { _eq: creationNumber },
};

const graphqlQuery = {
query: GetEvents,
variables: { where_condition: whereCondition },
};

const data = await queryIndexer<GetEventsQuery>({
aptosConfig,
query: graphqlQuery,
originMethod: "getEventsByCreationNumber",
});

return data.events;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's follow up with Events V2 support next

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh is Events v2 part of Alpha?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nvm lets skip

@gregnazario gregnazario force-pushed the jin/getChainTopUserTransactions_query branch 2 times, most recently from 6ccc28e to e17670e Compare October 13, 2023 21:42
Base automatically changed from jin/getChainTopUserTransactions_query to main October 13, 2023 21:43
@gregnazario gregnazario force-pushed the jin/getEventsByCreationNumber_indexer_query branch 2 times, most recently from 3b6c325 to 360d0d5 Compare October 13, 2023 21:46
@gregnazario gregnazario enabled auto-merge (rebase) October 13, 2023 21:46
@gregnazario gregnazario force-pushed the jin/getEventsByCreationNumber_indexer_query branch from 360d0d5 to 55dd6e8 Compare October 13, 2023 22:09
@gregnazario gregnazario merged commit 42b9256 into main Oct 13, 2023
5 checks passed
@gregnazario gregnazario deleted the jin/getEventsByCreationNumber_indexer_query branch October 13, 2023 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xmaayan 0xmaayan 0xmaayan left review comments

@heliuchuan heliuchuan heliuchuan left review comments

@gregnazario gregnazario gregnazario approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0xmigo @gregnazario @heliuchuan @0xmaayan