-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
[Example/CLI] Tic-tac-toe #18557
[Example/CLI] Tic-tac-toe #18557
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
examples/tic-tac-toe/cli/Cargo.lock
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that almost 7000 lines of this PR is just the lock file is ...insane. We shouldn't need this many deps to write a demo app.
In #18526 I left an experience report for the front-end, here's the same for the CLI! Mostly relevant to @wlmyng and @stefan-mysten as it may implications on the design of the new CLI or Rust GraphQL SDK: Compared to the TS SDK, the process is a little less polished here. I don't think that's a surprise to anyone, but hopefully we can tackle these paper cuts in our upcoming plans! ExamplesI relied heavily on @stefan-mysten's recent work in the CLI to act as a reference for how to build transactions. That's interesting for two reasons:
Transaction building
Coin SelectionThis logic is duplicated across multiple sites (in the transaction builder API, in the coin read API), with different sets of capabilities:
There should be one such API, it belongs in the SDK, and it needs to support gas smashing and sponsorship. Wallet vs SDK vs CLIIn all the points above, there is a component to the feedback that is "the right implementation exists somewhere, but it's probably in the wrong place, which makes it either impossible/hard to find, or not general enough for an SDK). There needs to be a clear separation between the SDK and Wallet libraries, and the CLI:
SigningThe wallet's Internal APIsIn a couple of places, we've exposed internal APIs because they offer the functionality that people needed, but we need to revisit these decisions -- we are often exposing too much or not enough by doing this. For example, by exposing our internal PTB building APIs, we are also exposing people to the This is helpful for ErrorsI made a typo ina function name in a PTB call and the only error I got back was CratesUsers need to bring in multiple of our crates to get started. This example required When it comes to the dependencies we are taking directly, these should either be built into an SDK and exported if they are expected to always be useful, or gated behind a feature flag (but again, still part of the SDK). When it comes to the large number of transitive dependencies, we need to do a better job keeping this number down. |
## Description Write a CLI to talk to the shared and multi-sig tic-tac-toe example Move packages. ## Test plan Manual testing: ``` $ tic-tac-toe view 0xab06bb92cc1e6c95f9e6eed7e6c01fade6bb5eaa241e2100eb54d914597295ed X | O | ---+---+--- O | X | ---+---+--- | | X X: <address> -> O: <address> GAME: 0xab06bb92cc1e6c95f9e6eed7e6c01fade6bb5eaa241e2100eb54d914597295ed ADMIN: <public key> X WINS! ``` Also, confirmed that the CLI works with the React front-end.
It needs to "float" on top of the deps that get pulled in by the workspace.
default.extend([".sui", "sui_config", "client.yaml"]); | ||
Some(default) | ||
}) else { | ||
bail!("Can't find Wallet config"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bail!("Can't find Wallet config"); | |
bail!("Cannot find wallet config in the known default directory {}", path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left a few comments.
nit #1:
let mut builder everywhere, but then let g = instead of let game
Not sure it saves a lot of typing, but it's probably a matter of preference. I always need to think hard for a few seconds to figure out what this single letter variable refers to.
nit #2: some places the error message ends with a . in others it doesn't.
} | ||
|
||
// (3) Deserialize contents | ||
let kind = match raw.type_.module.as_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly confused as to why this is matched on a string? Isn't there a well defined type/enum for possible modules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what type/enum you're referring to -- this is the module
for a StructTag
, it's an arbitrary Identifier
, and we are viewing it as a &str
to avoid having to create new Identifier
s to compare against it.
builder.programmable_move_call( | ||
self.package, | ||
raw.type_.module.clone(), | ||
Identifier::new("ended").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we know this game ended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function $PKG::$KIND::ended
is what tells us whether the game has ended (one of the players has won, or there is a draw), we are making this dev inspect call to re-use the logic in the Move package to answer this question for us.
|
||
let results = client | ||
.read_api() | ||
.dev_inspect_transaction_block( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dev_inspect_transaction_block
used before actually executing the last move to determine if this last move leads to the end of game?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dev inspect call is used to determine if the game has already ended. We run it every time we load the Game. When we make a move, we issue the transaction and then we read the Game state, so at that point we will see whether the game has ended, to display that.
let DryRunTransactionBlockResponse { effects, .. } = client | ||
.read_api() | ||
.dry_run_transaction_block(tx_data.clone()) | ||
.await | ||
.context("Error estimating gas budget")?; | ||
|
||
let gas_used = effects.gas_cost_summary(); | ||
let overhead = 1000 * gas_price; | ||
let net_used = gas_used.net_gas_usage(); | ||
let computation = gas_used.computation_cost; | ||
|
||
let budget = overhead + (net_used.max(0) as u64).max(computation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this tells me that the overhead function in the CLI dry_run
logic should be part of the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed -- we should expose gas estimation in the SDK.
async fn max_gas_budget(&self) -> Result<u64> { | ||
let client = self.client().await?; | ||
|
||
let cfg = client.read_api().get_protocol_config(None).await?; | ||
let Some(Some(SuiProtocolConfigValue::U64(max))) = cfg.attributes.get("max_tx_gas") else { | ||
bail!("Couldn't find max gas budget"); | ||
}; | ||
|
||
Ok(*max) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this one, should be part of an SDK.
async fn select_coins( | ||
&self, | ||
owner: SuiAddress, | ||
balance: u64, | ||
tx: &TransactionKind, | ||
) -> Result<Vec<ObjectRef>> { | ||
let client = self.client().await?; | ||
let coins = client.coin_read_api(); | ||
let exclude: Vec<_> = tx | ||
.input_objects()? | ||
.into_iter() | ||
.filter_map(|input| match input { | ||
InputObjectKind::ImmOrOwnedMoveObject((id, _, _)) => Some(id), | ||
InputObjectKind::MovePackage(_) => None, | ||
InputObjectKind::SharedMoveObject { .. } => None, | ||
}) | ||
.collect(); | ||
|
||
Ok(coins | ||
.select_coins(owner, None, balance as u128, exclude) | ||
.await? | ||
.into_iter() | ||
.map(|coin| coin.object_ref()) | ||
.collect()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be mistaking, but the wallet already provides this kind of functionality, so maybe you do not need to reimplement?
https://github.com/MystenLabs/sui/blob/main/crates/sui-sdk/src/wallet_context.rs#L180
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nice -- yes, I can use that (this conceptually belongs in the SDK though) -- the only potential concern is that it returns a single coin, so it doesn't support gas smashing, but that's not a big deal for this demo.
|
||
/// Delete a finished game. | ||
Delete { | ||
/// ID of the game to view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// ID of the game to view. | |
/// ID of the game to delete. |
ensure!( | ||
!*shared || !*multi_sig, | ||
"Cannot specify both shared and multi-sig" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always find this macro hard hard to reason about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably might be better to use https://docs.rs/clap/latest/clap/struct.Arg.html#method.conflicts_with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough review @stefan-mysten ! Will address these comments and re-request review.
let DryRunTransactionBlockResponse { effects, .. } = client | ||
.read_api() | ||
.dry_run_transaction_block(tx_data.clone()) | ||
.await | ||
.context("Error estimating gas budget")?; | ||
|
||
let gas_used = effects.gas_cost_summary(); | ||
let overhead = 1000 * gas_price; | ||
let net_used = gas_used.net_gas_usage(); | ||
let computation = gas_used.computation_cost; | ||
|
||
let budget = overhead + (net_used.max(0) as u64).max(computation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed -- we should expose gas estimation in the SDK.
async fn select_coins( | ||
&self, | ||
owner: SuiAddress, | ||
balance: u64, | ||
tx: &TransactionKind, | ||
) -> Result<Vec<ObjectRef>> { | ||
let client = self.client().await?; | ||
let coins = client.coin_read_api(); | ||
let exclude: Vec<_> = tx | ||
.input_objects()? | ||
.into_iter() | ||
.filter_map(|input| match input { | ||
InputObjectKind::ImmOrOwnedMoveObject((id, _, _)) => Some(id), | ||
InputObjectKind::MovePackage(_) => None, | ||
InputObjectKind::SharedMoveObject { .. } => None, | ||
}) | ||
.collect(); | ||
|
||
Ok(coins | ||
.select_coins(owner, None, balance as u128, exclude) | ||
.await? | ||
.into_iter() | ||
.map(|coin| coin.object_ref()) | ||
.collect()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, nice -- yes, I can use that (this conceptually belongs in the SDK though) -- the only potential concern is that it returns a single coin, so it doesn't support gas smashing, but that's not a big deal for this demo.
game: &game::Owned, | ||
game_ref: ObjectRef, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do refer to the same game. The first parameter is the contents of the game, read from RPC, and the second is its identity, which can be used to refer to it in a transaction.
This data exists together in the overall Game
struct, but we don't want to pass that in directly because then the function would need to check that the game it was given was an "owned" Game. Requesting the data in this way passes responsibility to the caller to do the check first and provide only the relevant information.
} | ||
|
||
// (3) Deserialize contents | ||
let kind = match raw.type_.module.as_str() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what type/enum you're referring to -- this is the module
for a StructTag
, it's an arbitrary Identifier
, and we are viewing it as a &str
to avoid having to create new Identifier
s to compare against it.
builder.programmable_move_call( | ||
self.package, | ||
raw.type_.module.clone(), | ||
Identifier::new("ended").unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function $PKG::$KIND::ended
is what tells us whether the game has ended (one of the players has won, or there is a draw), we are making this dev inspect call to re-use the logic in the Move package to answer this question for us.
|
||
let results = client | ||
.read_api() | ||
.dev_inspect_transaction_block( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dev inspect call is used to determine if the game has already ended. We run it every time we load the Game. When we make a move, we issue the transaction and then we read the Game state, so at that point we will see whether the game has ended, to display that.
}; | ||
|
||
let Some(raw) = raw.try_as_move() else { | ||
bail!("It is a package, not a object."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bail!("It is a package, not a object."); | |
bail!("It is a package, not an object."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All comments should be addressed now, could you take another look @stefan-mysten ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
## Description A README for the tic-tac-toe app example, derived from: https://github.com/MystenLabs/multisig_tic-tac-toe/blob/main/README.md And updates to the docs for the tic-tac-toe app example to reflect changes to modernise the guide and bring all its source code into the `sui` mono-repo. This is the last major example that lived in the `sui_programmability` directory. ## Test plan :eyes: ## Stack - #18525 - #18526 - #18557 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
…8595) ## Description Replace all references to Move packages in `sui_programmability` with equivalents in `./examples/move`, in preparation for deleting the programmability directory. In the process, I also: - removed the tic-tac-toe example from the Sui SDK, as it has been replaced by a more full-featured E2E example. - ported some modernised versions of the `basics` packages into the new `examples/move/basics` for use in tests. ## Test plan CI and, ``` sui$ cargo simtest ``` ## Stack - #18525 - #18526 - #18557 - #18558 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
…18609) ## Description Port over the following examples from sui_programmability/examples: - crypto/sources/ecdsa.move - crypto/sources/groth16.move - ~games/sources/drand_lib.move~ - ~games/sources/drand_based_lottery.move~ - ~games/sources/drand_based_scratch_card.move~ - games/sources/vdf_based_lottery.move Modernising and cleaning them up in the process: - Applying wrapping consistently at 100 characters, and cleaning up comments. - Removing unnecessary use of `entry` functions, including returning values instead of transfering to sender in some cases. - Using receiver functions where possible. - Standardising file order and adding titles for sections. - Standardising use of doc comments vs regular comments. - Using clever errors. This marks the final set of examples to be moved out of sui-programmability, which will then be deleted. ## Test plan ``` sui-framework-tests$ cargo nextest run -- run_examples_move_unit_tests ``` ## Stack - #18525 - #18526 - #18557 - #18558 - #18595 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
## Description Remove the `sui_programmability` folder as all examples have been ported and modernised to `examples/move`, or elsewhere. ## Test plan CI ## Stack - #18525 - #18526 - #18557 - #18558 - #18595 - #18609 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
## Description Write a CLI to talk to the shared and multi-sig tic-tac-toe example Move packages. ## Test plan Manual testing: ``` $ tic-tac-toe view 0xab06bb92cc1e6c95f9e6eed7e6c01fade6bb5eaa241e2100eb54d914597295ed X | O | ---+---+--- O | X | ---+---+--- | | X X: <address> -> O: <address> GAME: 0xab06bb92cc1e6c95f9e6eed7e6c01fade6bb5eaa241e2100eb54d914597295ed ADMIN: <public key> X WINS! ``` Also, confirmed that the CLI works with the React front-end. ## Stack - MystenLabs#18525 - MystenLabs#18526 --- ## 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:
## Description A README for the tic-tac-toe app example, derived from: https://github.com/MystenLabs/multisig_tic-tac-toe/blob/main/README.md And updates to the docs for the tic-tac-toe app example to reflect changes to modernise the guide and bring all its source code into the `sui` mono-repo. This is the last major example that lived in the `sui_programmability` directory. ## Test plan :eyes: ## Stack - MystenLabs#18525 - MystenLabs#18526 - MystenLabs#18557 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
…stenLabs#18595) ## Description Replace all references to Move packages in `sui_programmability` with equivalents in `./examples/move`, in preparation for deleting the programmability directory. In the process, I also: - removed the tic-tac-toe example from the Sui SDK, as it has been replaced by a more full-featured E2E example. - ported some modernised versions of the `basics` packages into the new `examples/move/basics` for use in tests. ## Test plan CI and, ``` sui$ cargo simtest ``` ## Stack - MystenLabs#18525 - MystenLabs#18526 - MystenLabs#18557 - MystenLabs#18558 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
…ystenLabs#18609) ## Description Port over the following examples from sui_programmability/examples: - crypto/sources/ecdsa.move - crypto/sources/groth16.move - ~games/sources/drand_lib.move~ - ~games/sources/drand_based_lottery.move~ - ~games/sources/drand_based_scratch_card.move~ - games/sources/vdf_based_lottery.move Modernising and cleaning them up in the process: - Applying wrapping consistently at 100 characters, and cleaning up comments. - Removing unnecessary use of `entry` functions, including returning values instead of transfering to sender in some cases. - Using receiver functions where possible. - Standardising file order and adding titles for sections. - Standardising use of doc comments vs regular comments. - Using clever errors. This marks the final set of examples to be moved out of sui-programmability, which will then be deleted. ## Test plan ``` sui-framework-tests$ cargo nextest run -- run_examples_move_unit_tests ``` ## Stack - MystenLabs#18525 - MystenLabs#18526 - MystenLabs#18557 - MystenLabs#18558 - MystenLabs#18595 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
## Description Remove the `sui_programmability` folder as all examples have been ported and modernised to `examples/move`, or elsewhere. ## Test plan CI ## Stack - MystenLabs#18525 - MystenLabs#18526 - MystenLabs#18557 - MystenLabs#18558 - MystenLabs#18595 - MystenLabs#18609 --- ## 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: --------- Co-authored-by: Ronny Roland <[email protected]>
Description
Write a CLI to talk to the shared and multi-sig tic-tac-toe example
Move packages.
Test plan
Manual testing:
Also, confirmed that the CLI works with the React front-end.
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.