-
Notifications
You must be signed in to change notification settings - Fork 11.7k
[static ptb] Initial check in #22256
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
| @@ -0,0 +1,78 @@ | |||
| // Copyright (c) Mysten Labs, Inc. | |||
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.
Can we not reuse the compiler's span library?
Not that I am particularly concerned, just curious.
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.
Compiler's Spanned<T> is a Loc + T where Loc is the byte offset/file hash etc. whereas this is a u16 + T so even though they have the same name they are quite different.
IMO this points to possibly renaming this to something a bit better. Possibly Indexed<T> or something like that?
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.
Yeah. I just named it spanned because of compiler reasons. I thought about Indexed but really hated it because it felt like some sort of mapping or database feeling word, being "indexed". Maybe I am over thinking it though
sui-execution/latest/sui-adapter/src/static_programmable_transactions/env.rs
Show resolved
Hide resolved
| @@ -0,0 +1,60 @@ | |||
| // Copyright (c) Mysten Labs, Inc. | |||
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 am a little confused about the naming -- in what sense is this static? Not trying to 🚲 🏠 too hard, just understand the idea.
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.
It is is "static" in the static typing sense. I think once there is an execution version cut though, this will get renamed to programmable_transactions. So happy to just _v2 it if y'all prefer
cgswords
left a comment
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 code mostly LGTM. I am sure as this matures we will all get more-comfortable with it and any small concerns I have now will go away. I think we should have @tzakian also look through it just to make sure.
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.
Left some comments as there are two main things to look at in typing, but otherwise I think good enough to land for now. I'll want to take a couple more in-depth passes over this before we would enable it anywhere, but that can be done while working on it -- for now I think this is a good starting point to jump off from.
sui-execution/latest/sui-adapter/src/programmable_transactions/context.rs
Outdated
Show resolved
Hide resolved
| vec![] | ||
| } | ||
| T::Command_::SplitCoins(_, coin, amounts) => { | ||
| // TODO should we just call a Move function? |
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.
Note if we plan on doing this that it will have some ramifications for linkage analysis (they'll be small/easy but just something worth noting possibly).
sui-execution/latest/sui-adapter/src/static_programmable_transactions/mod.rs
Show resolved
Hide resolved
| @@ -0,0 +1,78 @@ | |||
| // Copyright (c) Mysten Labs, Inc. | |||
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.
Compiler's Spanned<T> is a Loc + T where Loc is the byte offset/file hash etc. whereas this is a u16 + T so even though they have the same name they are quite different.
IMO this points to possibly renaming this to something a bit better. Possibly Indexed<T> or something like that?
sui-execution/latest/sui-adapter/src/static_programmable_transactions/typing/translate.rs
Outdated
Show resolved
Hide resolved
sui-execution/latest/sui-adapter/src/static_programmable_transactions/typing/translate.rs
Outdated
Show resolved
Hide resolved
## Description - Initial check in of static PTB rewrite - Main AST translation passes are complete - TODOs remain around linking and execution ## Test plan - Currently unused --- ## 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): - [ ] gRPC: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK:
…kage linkage (#8046) # Description of change Add additional signature and linkage checks. ## Links to any relevant issues fixes #8034 - Also partially pulled in changes from MystenLabs/sui#22362 for the used `InvalidLinkage` error, but no changes regarding `iota-rest-api` as it all will get refactored/renamed in MystenLabs/sui#20435. - Omitted changed touching **static-ptb** as those only get introduced in: MystenLabs/sui#22256 ## How the change has been tested - [ ] Basic tests (linting, compilation, formatting, unit/integration tests) - [ ] Patch-specific tests (correctness, functionality coverage) - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have checked that new and existing unit tests pass locally with my changes ### Release Notes - [x] Protocol: Adds a new protocol config options to enable additional signature and linkage checks. - [ ] Nodes (Validators and Full nodes): - [ ] Indexer: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: - [ ] REST API:
Description
Test plan
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.