Simplify predicate and script #19
Conversation
I'll definitely be scheduling a meeting to go over this. A LOT is changed in this repo now |
| } | ||
|
|
||
| // Get the data of a message input | ||
| pub fn input_message_data<T>(index: u64, offset: u64) -> T { |
There was a problem hiding this comment.
Same with this one, though it now returns a Bytes type. I refactored the Bridge-fungible-token contract to handle the new return type already here.
There was a problem hiding this comment.
How can I convert the Bytes to a u64?
There was a problem hiding this comment.
Hmm, we'd have to build a local conversion function based on the into() method for Bytes that converts to b256.
Ideally we would just impl TryInto () for Bytes, but we don't have that trait yet (blocked).
There was a problem hiding this comment.
@pixelcircuits this is what I was refering to here:
// NOTE: this cas be lossy! Added here as the From trait currently requires it,
// but the conversion from `Bytes` ->`b256` should be implemented as
// `impl TryFrom<Bytes> for b256` when the `TryFrom` trait lands:
// https://github.com/FuelLabs/sway/pull/3881
fn into(self) -> b256 {
let mut value = 0x0000000000000000000000000000000000000000000000000000000000000000;
let ptr = __addr_of(value);
self.buf.ptr().copy_to::<b256>(ptr, 1);
value
}
I think implementing something similar as a free function, i.e: fn into_u64(b: Bytes) -> u64; is the way to go for now.
There was a problem hiding this comment.
Bytes to b256 is already supported for into see here. Do you mean an into for Bytes to u64? I added a helper function to do just that, but were should it eventually get included in the sway std lib? The conversion to b256 is in the b256.sw file
There was a problem hiding this comment.
Eventually it should be added via implementing TryInto for u64, but this can't be done yet. The workaround is to impl From for Bytes and use the into()method, but this is also blocked atm.
| use std::collections::HashMap; | ||
|
|
||
| use fuel_core_interfaces::common::prelude::Word; | ||
| use fuels::prelude::*; |
There was a problem hiding this comment.
I would import explicitly i.e. comment out, build, see what it complains about and then add it back in manually.
There was a problem hiding this comment.
I tend to agree, but doesn't that defeat the whole reason we have the prelude?
| //referenced data (expected script hash) | ||
| //00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 | ||
| ] | ||
| .into_iter() |
There was a problem hiding this comment.
I'll put some comments here below so not to pollute the script itself.
- In general it'd be great to have those jump destination 'addresses' (18, 16, 11) as constants as well, with names which match the comment before them. And if possible, to do a check after collecting them that each offset points the the instruction you expect it to. That might be overkill for a script which doesn't change often, but a thought. The 19 could be verified very easily too.
- The
addi(EXPECTED_INPUT_TYPE, ZERO, INPUT_MESSAGE_TYPE)could just be amovi. You might've copied that from the compiler output. We hadADDIbeforeMOVIand some parts of the compiler haven't been updated. - Similarly the
op::jnei(INPUT_MSG_DATA_LEN, ZERO, 18)could bejnzi. - You're assuming that
INPUT_INDEXstarts at at least 2, although I guess if it's 1 then it'll be checking the first input which will never have zero length? And if it's 0 then thesubiwill underflow. - I've been trying to think of a way to shorten it by optimising the control flow but I don't think there's a way to remove any more instructions. 👍 Although I did just notice that you could use the heap instead of the stack for the script hash buffer, which, once the new
ALOCsemantics are merged, would mean you could just use$hpinstead ofSCRIPT_HASH_PTRand that would remove onemoveinstruction. 🙂
There was a problem hiding this comment.
Great thinking with using the heap vs the stack!
There was a problem hiding this comment.
I spoke too soon... there is no immediate version of ALOC like there is CFEI so it would require at least 2 instructions to increase the heap.
There was a problem hiding this comment.
I moved the jump instruction values to constants since some of them are referenced more than once. We could add a check for things like the value for the end of the program code before adding referenced data bits, but we effectively already are indirectly through the tests (particularly the test where everything has to work correctly).
There was a problem hiding this comment.
once the new ALOC semantics are merged
Using the heap may be an optimization that has to wait, not sure when it will land.
Using jnzi sounds like an easy win though.
I don't have anything to add; @otrho was too thorough :)
There was a problem hiding this comment.
I spoke too soon... there is no immediate version of
ALOClike there isCFEIso it would require at least 2 instructions to increase the heap.
Ahh, yeah. Bugger. OK, I think it has been golfed all the way.
These tests ensure the bytecode won't change, and also provides an easy way to reference the serialized bytecode.
pixelcircuits
deleted the
18-update-message-to-contract-predicate-with-new-message-retry-spec
branch
No description provided.