Skip to content
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

fix: MessageOut encode/decode for fuel-core v0.18 #1090

Merged
merged 5 commits into from
Jul 7, 2023
Merged

Conversation

luizstacio
Copy link
Member

@luizstacio luizstacio commented Jul 7, 2023

Fix message decode for fuel-core 0.18.3.

This PR, Includes a temporary solution for a bug found on the fuel-vm side. The fix for this issue would generate a break change. This is why we are proposing the temp solution directly on the SDK level for now.

With this solution, the MessageReceiptOut encoder will not match the encoding results, as our encode works correctly according to the specs.

Current message out bytes returned from fuel-core

// Type (8)
0, 0, 0, 0, 0, 0, 0, 10,
// Sender (32)
121, 108, 232, 48, 254, 183, 45, 178, 92, 254, 191,
36, 78, 227, 74, 129, 232, 160, 62, 188, 91, 62, 112, 16, 197, 212, 244, 74,
4, 245, 121, 59,
// Recipient (32)
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 95, 200, 211, 38, 144,
204, 145, 212, 195, 157, 157, 58, 188, 189, 22, 152, 159, 135, 87, 7,
// Amount (8)
0, 0, 0, 0, 0, 0, 0, 0,
// Nonce (32)
135, 203, 218, 102, 65, 49, 142, 139, 38, 239, 229, 156, 217,
68, 163, 211, 24, 172, 31, 227, 68, 99, 27, 104, 157, 201, 215, 32, 160, 82,
2, 11,
// DataLength (8)  (This should be the length of the data but is returning the wrong size) <-----
0, 0, 0, 0, 0, 0, 0, 104,
// Digest (32)
131, 27, 27, 49, 15, 91, 218, 138, 171, 46,
201, 165, 84, 127, 68, 63, 241, 10, 47, 69, 101, 192, 147, 41, 62, 212, 223,
202, 129, 74, 143, 24,
// DataLength (16) (This is not on specs and also has a wrong length of 16 bytes) <-----
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 100,
// Data (The message sent from the contract only have 100 bytes but because of the issue on the fuel-core this current length returned is 104)
83, 239, 20, 97, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 60, 68, 205, 221,
182, 169, 0, 250, 43, 88, 93, 210, 153, 224, 61, 18, 250, 66, 147, 188, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 43, 13, 54, 250, 205, 97, 183, 28, 192, 90, 184, 243,
210, 53, 94, 195, 99, 28, 13, 213, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 138, 199, 35, 4, 137, 232, 0, 0, 0, 0, 0, 0,

@github-actions
Copy link
Contributor

github-actions bot commented Jul 7, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
84.65% (-0.46% 🔻)
4622/5460
🟡 Branches
63.65% (+0.02% 🔼)
653/1026
🟡 Functions
69.76% (-0.09% 🔻)
736/1055
🟢 Lines
84.82% (-0.47% 🔻)
4459/5257
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / input.ts
78.38% (-2.18% 🔻)
78.57% (+3.57% 🔼)
87.5%
78.38% (-2.18% 🔻)
🟢
... / receipt.ts
88.56% (-6.42% 🔻)
78.57% (-6.04% 🔻)
89.47% (-2.63% 🔻)
88.56% (-6.42% 🔻)

Test suite run success

1001 tests passing in 180 suites.

Report generated by 🧪jest coverage report action from f53da43

@luizstacio luizstacio self-assigned this Jul 7, 2023
@luizstacio luizstacio marked this pull request as ready for review July 7, 2023 17:46
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🪄

@danielbate
Copy link
Contributor

Only request is could the PR name be more specific to message encoding/decoding so it can be easily found when this breaks again when it's fixed in fuel-core

@luizstacio luizstacio changed the title fix: data fields and messageId fix: MessageOut encode/decode for fuel-core v0.18 Jul 7, 2023
@luizstacio
Copy link
Member Author

Only request is could the PR name be more specific to message encoding/decoding so it can be easily found when this breaks again when it's fixed in fuel-core

You think now is better?

[decoded, o] = new B256Coder().decode(data, o);
const digest = decoded;
[decoded, o] = new ByteArrayCoder(len).decode(data, o);
// TODO: remove this once fuel-vm is fixed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, is there an issue open on the fuel-vm repo that we can subscribe to in order to know when this gets fixed i.e. when we need to work on a fix here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw that FuelLabs/fuel-core#1240 was closed. Do we need to work on a fix now? @luizstacio

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should address this at: #1112

@luizstacio luizstacio merged commit c07912c into master Jul 7, 2023
@luizstacio luizstacio deleted the ls/fix/messages branch July 7, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants