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(rust-sdk): Simulate WaitForLocalExecution #18996

Merged
merged 4 commits into from
Aug 16, 2024
Merged

feat(rust-sdk): Simulate WaitForLocalExecution #18996

merged 4 commits into from
Aug 16, 2024

Conversation

amnn
Copy link
Contributor

@amnn amnn commented Aug 15, 2024

Description

If transaction execution requires waiting for local execution, then simulate it by polling, to account for the fact that fullnodes will soon start to ignore this parameter.

Test plan

Run the programmable transaction SDK example:

sui-sdk$ cargo run --example programmable_transactions_api

Run the tic-tac-toe E2E example:

examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- new $(sui client active-address)
                                  |   |
                               ---+---+---
                                  |   |
                               ---+---+---
                                  |   |

 -> X: <ADDRESS>
    O: <ADDRESS>
 GAME: <GAME>

examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- move -r 0 -c 0 $GAME

...

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:
  • GraphQL:
  • CLI:
  • Rust SDK: Adds support for simulating WaitForLocalExecution in the client, using polling, as the flag will be ignored by fullnodes shortly.
  • REST API:

## Description

If transaction execution requires waiting for local execution, then
simulate it by polling, to account for the fact that fullnodes will soon
start to ignore this parameter.

## Test plan

Run the programmable transaction SDK example:

```
sui-sdk$ cargo run --example programmable_transactions_api
```

Run the tic-tac-toe E2E example:

```
examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- new $(sui client active-address)
                                  |   |
                               ---+---+---
                                  |   |
                               ---+---+---
                                  |   |

 -> X: <ADDRESS>
    O: <ADDRESS>
 GAME: <GAME>

examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- move -r 0 -c 0 $GAME

...
```
@amnn amnn requested review from lxfind, stefan-mysten, hayes-mysten and a team August 15, 2024 12:02
@amnn amnn self-assigned this Aug 15, 2024
Copy link

vercel bot commented Aug 15, 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 Aug 15, 2024 8:35pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Aug 15, 2024 8:35pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Aug 15, 2024 8:35pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Aug 15, 2024 8:35pm

Copy link
Contributor Author

@amnn amnn left a comment

Choose a reason for hiding this comment

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

Testing the failure case (fail to confirm and timeout) is a little tricky, let me think on how to do that.

@@ -180,6 +180,10 @@ impl SuiTransactionBlockResponseOptions {
}
}

#[deprecated(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is more we could do with the flags here (the criteria for requiring effects etc seems too generous), but I am resisting here seeing as we will be moving to a new SDK soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does default_execution_request_type need to be updated?

Copy link
Contributor Author

@amnn amnn Aug 15, 2024

Choose a reason for hiding this comment

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

I considered it but decided not to because I'm afraid it would open a can of worms that we would then need to deal with in the near term, even though the underlying issues would disappear with the GraphQL Rust SDK, namely:

  • Why do we set WaitForLocalExecution on the basis of require_effects instead of ...require_local_execution?
  • Why do we need to do anything special to get the raw effects?

It seems at some point these two things got conflated, and now it's easier to keep them coupled until we introduce GraphQL Rust SDK (or we get rid of WaitForLocalExecution as a flag on the client), because otherwise we might break the experience for people who are implicitly relying on read-write consistency they got from requesting just effects.

if let Ok(poll_response) = self
.api
.http
.get_transaction_block(*tx.digest(), Some(options.clone()))
Copy link
Contributor

Choose a reason for hiding this comment

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

So I clearly have some gaps in my Transaction related functionality understanding, but once a transaction it's built, a digest is generated. That's why we can do polling, because we already know the digest. So the digest cannot change once the tx is built, correct? I thought digest is dependent on other factors (e.g., time when tx gets accepted)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are multiple kinds of digest -- the transaction digest, which we are using here, is derived from the transaction input so that is not going to change. There is also an effects digest, which is a hash of the transaction effects, so we will not know that until we get the full effects.

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.

Smooth! Like it. 🕺

crates/sui-sdk/src/apis.rs Outdated Show resolved Hide resolved
@amnn
Copy link
Contributor Author

amnn commented Aug 15, 2024

Testing the failure case (fail to confirm and timeout) is a little tricky, let me think on how to do that.

In the end, I tested this by modifying the fullnode implementation of get_transaction_block to introduce a 120 second delay, and then running the programmable_transactions_api example against localnet to confirm that it completes within 60s because of the timeout.

@amnn amnn merged commit 396ecf1 into main Aug 16, 2024
48 checks passed
@amnn amnn deleted the amnn/rust-sdk-poll branch August 16, 2024 09:37
tx-tomcat pushed a commit to tx-tomcat/sui-network that referenced this pull request Aug 27, 2024
## Description

If transaction execution requires waiting for local execution, then
simulate it by polling, to account for the fact that fullnodes will soon
start to ignore this parameter.

## Test plan

Run the programmable transaction SDK example:

```
sui-sdk$ cargo run --example programmable_transactions_api
```

Run the tic-tac-toe E2E example:

```
examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- new $(sui client active-address)
                                  |   |
                               ---+---+---
                                  |   |
                               ---+---+---
                                  |   |

 -> X: <ADDRESS>
    O: <ADDRESS>
 GAME: <GAME>

examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- move -r 0 -c 0 $GAME

...
```

---

## 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: 
- [ ] GraphQL: 
- [ ] CLI: 
- [X] Rust SDK: Adds support for simulating `WaitForLocalExecution` in
the client, using polling, as the flag will be ignored by fullnodes
shortly.
- [ ] REST API:
suiwombat pushed a commit that referenced this pull request Sep 16, 2024
## Description

If transaction execution requires waiting for local execution, then
simulate it by polling, to account for the fact that fullnodes will soon
start to ignore this parameter.

## Test plan

Run the programmable transaction SDK example:

```
sui-sdk$ cargo run --example programmable_transactions_api
```

Run the tic-tac-toe E2E example:

```
examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- new $(sui client active-address)
                                  |   |
                               ---+---+---
                                  |   |
                               ---+---+---
                                  |   |

 -> X: <ADDRESS>
    O: <ADDRESS>
 GAME: <GAME>

examples/tic-tac-toe/cli$ env $(cat testnet.env) \
  cargo run -- move -r 0 -c 0 $GAME

...
```

---

## 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: 
- [ ] GraphQL: 
- [ ] CLI: 
- [X] Rust SDK: Adds support for simulating `WaitForLocalExecution` in
the client, using polling, as the flag will be ignored by fullnodes
shortly.
- [ ] 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.

4 participants