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
Add std::tx::get_predicate_data
#1984
Conversation
test/src/sdk-harness/test_projects/predicate_data_struct/mod.rs
Outdated
Show resolved
Hide resolved
test/src/sdk-harness/test_projects/predicate_data_struct/mod.rs
Outdated
Show resolved
Hide resolved
r1: u64 | ||
}; | ||
|
||
let predicate_data_ptr = is + predicate_code_length; |
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 should be
let predicate_data_ptr = is + predicate_code_length; | |
let predicate_data_ptr = is + predicate_code_length * 4; |
The fact that it's not means there's a bug in the SDKs where they're not dividing the lengths by 4 to get the number of instructions instead of the number of bytes, as described in the specs.
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.
Got it @adlerjohn
So to be clear, on the SDK side which statement is true:
- divide
predicate code
length by 4 - divide
predicate data
length by 4 - divide both
predicate code
length andpredicate data
by 4
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.
Only (1). Code length is in instructions (each instruction is 4 bytes), data length is in bytes.
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.
So does this still need to be resolved somewhere? Is there an open issue to fix the code length thing?
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 investigating one more thing on the SDK side so will respond once I have more information
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.
On the TypeScript SDK side, there is immediate failure if the predicate code length is divided by 4. On the Rust SDK side most of the work is offloaded out of the SDK into Input::coin_predicate
- see here.
I think we should add this helper as is currently and add some issues to investigate further. Allowing this to merge will cleanup unit tests on TS and RS side of things and permit a good initial level of Predicate
support, and any changes that need to occur in regards to dividing by 4 in here can be address fairly easily downstream.
…lLabs/sway into feature/stdlib-tx-get-predicate-data
@adlerjohn @otrho can we re-review based on my comments? Getting this particular helper in place would be wonderful to reduce copy pasted code across the Rust and TypeScript Sway programs (used by unit tests of Predicate) where I've duplicated this helper logic. The helper is working as-is with Rust and TS SDK, changing it to divide by 4 causes issues on the GraphQL server. I think the vm team may have to investigate further on the issue, and at that point, updating this helper and the SDKs would be simple changes. |
Closes #1981