feat: parse op_return #3
Conversation
There was a problem hiding this comment.
Great job, special mention for the tests, it must have been painful. The code seems to be very similar to the Ord spec, which is not optimal for performances but is perfect for maintainability, so let's keep it that way unless we find out it really is too expensive. If that's the case a simple way to optimize would be to just panic instead of using flaws (and just not support transactions that contain one error). When you add the test for decode_integer(s), please add a comment with the gas result.
Note: Edicts (transfers) don't seem to be decoded correctly. We can use Hiro to write our own tests but I reported the issues on Ord github, see ordinals/ord#4122
src/parser.cairo
Outdated
| loop { | ||
| if current_index >= pubscript.len() { |
There was a problem hiding this comment.
instead of checking if the index is > to the len, could we pop_front and match the first element? Because pubscript[current_index] is actually or shortcode for:
if current_index >= pubscript.len() {
panic
else {
return the value
}
So we actually do the same check twice.
src/parser.cairo
Outdated
| // * 256 is << 8z | ||
| let size: u32 = ((pubscript[current_index]).into() | ||
| // so it doesn't exceed the max value | ||
| | ((pubscript[current_index + 1]).into() * 256) |
There was a problem hiding this comment.
what's the type of 256 here? I guess it should be u32 but It would be better to specify 256_u32 just to make sure we don't get something weird if the compiler updates the automatic type resolution
src/parser.cairo
Outdated
| let mut i = 0; | ||
|
|
||
| loop { | ||
| if i >= payload.len() { |
There was a problem hiding this comment.
Same advice, let's match on pop_front if possible
src/parser.cairo
Outdated
| if k >= 4 || j + k >= payload.len() { | ||
| break; | ||
| } | ||
| chunk.append(*payload[j + k]); |
There was a problem hiding this comment.
It doesn't look like you need to backtrack here, so you can probably just pop_front 4 times to build a chunk
| use alexandria_math::pow; | ||
| use alexandria_data_structures::byte_array_ext::ByteArrayIntoArrayU8; | ||
|
|
||
| pub fn decode_integers(payload: ByteArray) -> Result<Array<u128>, ByteArray> { |
There was a problem hiding this comment.
I think this function will be expensive. I see a few way to optimize it but if we really want to do it well it won't be trivial. Let's not focus on it at the moment, but please add some tests just so we can safely try faster implementations in the future.
| if pubscript[0] == OP_RETURN && pubscript[1] == OP_13 { | ||
| // load payload | ||
| let payload = match load_payload(pubscript.clone()) { |
There was a problem hiding this comment.
I have this error when I don't clone : Cannot desnap a non copyable type. note: Trait has no implementation in context: core::traits::Copy::<core::byte_array::ByteArray>.
src/parser.cairo
Outdated
| let size: u32 = opcode.into(); | ||
| let mut i: usize = 0; | ||
| loop { | ||
| if i >= size { |
There was a problem hiding this comment.
If I understand we only increment i by 1, so we can check i == size to break (it's much cheaper)
src/parser.cairo
Outdated
| }; | ||
| let mut i: usize = 0; | ||
| loop { | ||
| if i >= size { |
src/parser.cairo
Outdated
|
|
||
| let mut i: usize = 0; | ||
| loop { | ||
| if i >= size { |
src/parser.cairo
Outdated
|
|
||
| let mut i: usize = 0; | ||
| loop { | ||
| if i >= size { |
Th0rgal
added
❌ Change request
irisdv
added
🔥 Ready for review
Close #1