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

feat: add Vec support as an Input #497

Merged
merged 25 commits into from
Sep 11, 2022
Merged

feat: add Vec support as an Input #497

merged 25 commits into from
Sep 11, 2022

Conversation

camsjams
Copy link
Contributor

@camsjams camsjams commented Sep 9, 2022

Closes #434

Here is a basic example of my thought process, also see #434 for greater details
Blank diagram

@camsjams camsjams self-assigned this Sep 9, 2022
@camsjams camsjams requested a review from a team September 9, 2022 18:20
@camsjams camsjams marked this pull request as ready for review September 9, 2022 18:36
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 90.01% 3298/3664
🟡 Branches 70.66% 614/869
🟢 Functions 86.91% 664/764
🟢 Lines 89.85% 3161/3518
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / vec.ts
83.33% 25% 83.33% 82.61%
🟡
... / LogReader.ts
72.22% 100% 50% 72.22%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / utilities.ts
100%
90% (-10% 🔻)
100% 100%
🟢
... / abi-coder.ts
95.6%
89.74% (-1.43% 🔻)
100% 95.45%
🟢
... / invocation-scope.ts
100%
57.14% (-2.86% 🔻)
100% 100%

Test suite run success

503 tests passing in 44 suites.

Report generated by 🧪jest coverage report action from 7827436

packages/abi-coder/src/abi-coder.ts Outdated Show resolved Hide resolved
packages/abi-coder/src/abi-coder.ts Show resolved Hide resolved
packages/abi-coder/src/abi-coder.ts Outdated Show resolved Hide resolved
packages/abi-coder/src/coders/vec.ts Outdated Show resolved Hide resolved
packages/providers/src/LogReader.ts Show resolved Hide resolved
@camsjams camsjams mentioned this pull request Sep 10, 2022
2 tasks
@camsjams camsjams requested review from luizstacio and a team September 10, 2022 07:24
WORD_SIZE + // Byte price
WORD_SIZE + // Maturity
WORD_SIZE + // Script size
// WORD_SIZE + // Script data size
Copy link
Contributor

Choose a reason for hiding this comment

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

is this supposed to be commented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question!

The pointer for script data used in vector location is built upon vm TX memory offset + script bytes length (padded to word size) + base transaction size (this var) + script data length

In my case this particular calculation was off by 8 bytes, so my assumption was that this line of code was the issue because script data size is calculated dynamically.

I may be way off base here, would love others to weigh in @luizstacio @adlerjohn @digorithm

Copy link
Member

Choose a reason for hiding this comment

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

The correct field to remove is WORD_SIZE + // Byte price which was removed on fuel-core v0.10.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated - thanks @luizstacio !

QuinnLee
QuinnLee previously approved these changes Sep 10, 2022
@@ -4,7 +4,7 @@
"declaration": true,
"declarationMap": true,
"esModuleInterop": true,
"lib": ["ES2020"],
"lib": ["ES2021"],
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to change this? Any impact on the build also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to address the issue in LogReader. I had some notes on it there regarding the ts-lint ignore. I'm happy to revert this and reinstate the comment there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luizstacio see here for motivation of this change #497 (comment)

WORD_SIZE + // Byte price
WORD_SIZE + // Maturity
WORD_SIZE + // Script size
// WORD_SIZE + // Script data size
Copy link
Member

Choose a reason for hiding this comment

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

The correct field to remove is WORD_SIZE + // Byte price which was removed on fuel-core v0.10.

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.

Add Vec<T> support as an Input
3 participants