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

core: event sending module relocation #19672

Merged
merged 4 commits into from
Oct 3, 2024
Merged

core: event sending module relocation #19672

merged 4 commits into from
Oct 3, 2024

Conversation

amnn
Copy link
Member

@amnn amnn commented Oct 2, 2024

Description

Make it so that the module that is associated as the "sending module" for an event is relocated by linkage. This fixes an issue (captured in a regression test) where if the function that is called by the PTB is from some upgraded version of the package, it cannot be found because the Event's package ID still points to the original version of the package.

Test plan

Introduced a regression test -- before the change, the "sending module" in the response returned null.

sui$ cargo nextest run -p sui-graphql-e2e-tests -- sending_module

Stack


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol: Protocol version bump to 62, changing the semantics around the package/module in the PTB that the event originated from.
  • Nodes (Validators and Full nodes):
  • Indexer:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:
  • REST API:

@amnn amnn requested review from tnowacki and tzakian October 2, 2024 22:21
@amnn amnn self-assigned this Oct 2, 2024
Copy link

vercel bot commented Oct 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 10:43pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2024 10:43pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2024 10:43pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Oct 3, 2024 10:43pm

Copy link
Contributor

@tzakian tzakian left a comment

Choose a reason for hiding this comment

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

Changes look good to me

Copy link
Contributor

@stefan-mysten stefan-mysten left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix here @amnn

amnn added 4 commits October 3, 2024 23:33
## Description

Avoid duplicating fields for configs that are accepted as flags from the
command-line and can also be read from TOML file. Use the same struct
for both purposes but deocrate it with both `clap` and `serde` (or
`GraphQLConfig`) annotations so that it can serve both purposes.

## Test plan

This change should preserve the config/command-line interface -- checked
by running the GraphQL service with the same invocation, referencing all
flags, before and after the change.
## Description

Add a flag to optionally skip database migration compatibility checks.
This is helpful when trying to connect a local build up to a production
database to test a specific change, even if you are aware that other
queries may not be compatible.

To accommodate this change, the compatibility check was also moved into
`ServerBuilder` where the config is readily available. This also
slightly simplifies the `Server` itself, which no longer needs to hold
onto its own instance of the `Db`.

## Test plan

Connected a local build of GraphQL to the production DB, which it is not
compatible with.
## Description

Make it so that the module that is associated as the "sending module"
for an event is relocated by linkage. This fixes an issue (captured in a
regression test) where if the function that is called by the PTB is from
some upgraded version of the package, it cannot be found because the
Event's package ID still points to the original version of the package.

## Test plan

Introduced a regression test -- before the change, the "sending module"
in the response returned `null`.

```
sui$ cargo nextest run -p sui-graphql-e2e-tests -- sending_module
```
@amnn amnn force-pushed the amnn/gql-skip-compat branch from 84c2a80 to f39f977 Compare October 3, 2024 22:39
@amnn amnn force-pushed the amnn/core-ev-fix branch from 67b77fe to bd9f067 Compare October 3, 2024 22:39
@amnn amnn mentioned this pull request Oct 3, 2024
8 tasks
Base automatically changed from amnn/gql-skip-compat to main October 3, 2024 23:12
@amnn amnn merged commit 59f115b into main Oct 3, 2024
45 of 47 checks passed
@amnn amnn deleted the amnn/core-ev-fix branch October 3, 2024 23:12
amnn added a commit that referenced this pull request Oct 10, 2024
## Description

Add the ability to fetch the transaction block that emitted the event.

## Test plan

New E2E tests:

```
sui$ cargo nextest run -p sui-graphql-e2e-tests
sui$ cargo nextest run -p sui-graphql-rpc -- test_schema_sdl_export
sui$ cargo nextest run -p sui-graphql-rpc --features staging -- test_schema_sdl_export
```

## Stack

- #19670 
- #19671 
- #19672 

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [x] GraphQL: Add `Event.transactionBlock` for fetching the transaction
that emitted the event, as long as the event is indexed (not just
executed).
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
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