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 main arg support to Scripts #745

Merged
merged 63 commits into from
Feb 23, 2023
Merged

feat!: add main arg support to Scripts #745

merged 63 commits into from
Feb 23, 2023

Conversation

camsjams
Copy link
Contributor

@camsjams camsjams commented Jan 20, 2023

closes #698

Follow-up in here for additional testing: #757

@camsjams camsjams self-assigned this Jan 20, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
94.74% (+8.72% 🔼)
4360/4602
🟢 Branches
82.7% (+15.45% 🔼)
779/942
🟢 Functions
91.91% (+12.7% 🔼)
864/940
🟢 Lines
94.62% (+8.72% 🔼)
4189/4427
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / getDecodedLogs.ts
100% 100% 100% 100%
🟢
... / contract-factory.ts
86.49% 66.67% 83.33% 86.49%
🟢
... / index.ts
100% 100% 66.67% 100%
🟢
... / utils.ts
91.67% 50% 100% 90.91%
🟢
... / constants.ts
100% 100% 100% 100%
🟢
... / errors.ts
100% 83.33% 100% 100%
🟢
... / invocation-scope.ts
100% 57.14% 100% 100%
🟢
... / base-invocation-scope.ts
100% 92.86% 100% 100%
🟢
... / contract-call-script.ts
96.15% 87.5% 100% 96%
🟢
... / multicall-bin.ts
100% 100% 100% 100%
🟢
... / script-request.ts
84.44% 60% 88.89% 84.44%
🟢
... / invocation-results.ts
95.56% 75% 100% 95.45%
🟢
... / multicall-scope.ts
100% 100% 100% 100%
🟢
... / contract.ts
95.45% 85.71% 100% 95.24%
🟢
... / script-invocation-scope.ts
91.67% 100% 80% 91.67%
🟢
... / utils.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / index.ts
100% 100% 100% 100%
🟢
... / ExampleContractAbi__factory.ts
83.33% 100% 50% 83.33%

Test suite run success

728 tests passing in 99 suites.

Report generated by 🧪jest coverage report action from 28c6871

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Great work! 👏 🤘

I've left some comments, most coming from my yesterday's draft: 6a21da2.

In the draft, I tried sketching a txParams() method as the Contract class does.

Do you think it would make sense to have such a method?

packages/script/src/script-factory.ts Outdated Show resolved Hide resolved
packages/script/src/script-factory.ts Outdated Show resolved Hide resolved
packages/script/src/callScript.ts Outdated Show resolved Hide resolved
packages/script/src/callScript.ts Outdated Show resolved Hide resolved
packages/abi-coder/src/interface.ts Outdated Show resolved Hide resolved
@camsjams
Copy link
Contributor Author

camsjams commented Jan 20, 2023

Do you think it would make sense to have such a method?

@arboleya yea I think I'd like to refactor this PR a little bit to make it more like Contract DX

I'm going to mark as a draft

@camsjams camsjams marked this pull request as draft January 20, 2023 19:17
@camsjams camsjams marked this pull request as ready for review January 30, 2023 09:50
@camsjams camsjams requested a review from arboleya January 30, 2023 09:50
@camsjams
Copy link
Contributor Author

@arboleya I reworked this to make Scripts usage available as a method of a Wallet. This is similar to how Predicates work as generally using a script will always require funds to complete, so its a function of having available assets to spend in the transaction.

@camsjams camsjams changed the title feat: add main arg support to Scripts feat!: add main arg support to Scripts Feb 22, 2023
@camsjams camsjams requested a review from Torres-ssf February 22, 2023 16:31
Torres-ssf
Torres-ssf previously approved these changes Feb 22, 2023
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

Good stuff -- looks pretty clean to me.

packages/program/src/script-request.ts Show resolved Hide resolved
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

The addition of the program package couldn't feel more appropriate. 😌

Copy link
Contributor

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

Brilliant!

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 support for Script main() arguments
6 participants