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: Parse Logs and LogData #517

Merged
merged 1 commit into from
Sep 24, 2022
Merged

feat: Parse Logs and LogData #517

merged 1 commit into from
Sep 24, 2022

Conversation

QuinnLee
Copy link
Contributor

@QuinnLee QuinnLee commented Sep 23, 2022

Closes #491

First, pass on log and log data.

I tried to refactor the logic to decode these values, but it quickly grew out of scope. I think the good place to start refactors are on the ABI, Interface and InvocationResults.

WIP - More tests while getting this into review

@@ -234,20 +234,23 @@ describe('Coverage Contract', () => {
});

it('should test u8 vector input', async () => {
const { value, transactionResult } = await contractInstance.functions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update the test to read the logs

@github-actions
Copy link
Contributor

github-actions bot commented Sep 23, 2022

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 90.14% 3308/3670
🟡 Branches 70.74% 619/875
🟢 Functions 87.3% 667/764
🟢 Lines 89.97% 3167/3520
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / index.ts
100% 100%
53.33% (-6.67% 🔻)
100%
🟢
... / interface.ts
90%
76.47% (+2.4% 🔼)
92.31% (+1.4% 🔼)
89.86%

Test suite run success

503 tests passing in 44 suites.

Report generated by 🧪jest coverage report action from de884eb

@QuinnLee QuinnLee marked this pull request as ready for review September 23, 2022 02:22
@QuinnLee QuinnLee requested review from luizstacio, camsjams and LuizAsFight and removed request for luizstacio and camsjams September 23, 2022 02:22
this.fragments = coerceFragments(ABI.unflatten(jsonAbi));

this.types = this.abi ? this.abi.types : [];
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 isn't used, but my ocd kicked in and I added it. It can be removed

Comment on lines +84 to +85
parseLoggedType(loggedType: JsonFlatAbiFragmentLoggedType): JsonAbiFragmentType {
return ParamType.fromObject(this.parseInput(loggedType.loggedType) as JsonFragmentType);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was much easier to reuse the current logic - but this should be cleaned up asap

Copy link
Contributor

@camsjams camsjams left a comment

Choose a reason for hiding this comment

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

Looks good, just parking as requesting changes per the flat abi question

@@ -234,20 +234,23 @@ describe('Coverage Contract', () => {
});

it('should test u8 vector input', async () => {
const { value, transactionResult } = await contractInstance.functions
const { value, logs } = await contractInstance.functions
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

return this.loggedTypes.map((loggedType) => ({
...loggedType,
abiFragmentType: [this.parseLoggedType(loggedType)],
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the non-flat ABI is no more? @luizstacio maybe I misunderstood

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 is correct. My understanding is that we unflatten it to make it work with the code. We really need to refactor the how the coders and ABI interact because this is where the tech debt is

Copy link
Member

Choose a reason for hiding this comment

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

The idea behind keeping the nonthe flat abi, was to keep possible partial definition of functions using objects. But once logs and other things are going to require types. We should remove it.

@camsjams camsjams self-requested a review September 24, 2022 01:31
@camsjams camsjams merged commit 6403076 into master Sep 24, 2022
@camsjams camsjams deleted the ql/parse-log-logData branch September 24, 2022 01:31
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.

Parse logs and events from contract calls
3 participants