Skip to content

streamline stack item types in Neo3EventListener#48

Merged
luc10921 merged 5 commits into
mainfrom
streamline-eventlistener
Oct 6, 2025
Merged

streamline stack item types in Neo3EventListener#48
luc10921 merged 5 commits into
mainfrom
streamline-eventlistener

Conversation

@ixje

@ixje ixje commented Oct 6, 2025

Copy link
Copy Markdown
Member

It started by realizing I could not do

import {NeonEventListener, NeonParser} from '@cityofzion/neon-dappkit'
...
const txid ='0x2a538a9651056b632625a253406ab554459c6d5e83bf341c7eb2ab66e216d3a6'
const eventListener = new NeonEventListener('https://mainnet1.neo.coz.io:443')
const log = await eventListener.waitForApplicationLog(txid)
NeonParser.parseRpcResponse(log.executions[0].stack![0])

but I had to use

NeonParser.parseRpcResponse(log.executions[0].stack![0] as RpcResponseStackItem)

Which made no sense to me as there is only one stack item in the VM.

Upon closer inspection I realized that Neo3EventListener had it's own stack item interface (Neo3StackItem) where Neo3Parser, Neo3Invoker and TypeChecker all use RpcResponseStackItem. I visualized that state below

image

Note that InvokeResult is also missing notifications[] and the Neo3ApplicationLog is missing the exception field.

This PR addresses the issue by ensuring that Neo3EventListener uses the same stack item and notification types as elsewhere. Specifically as shown in the image below

image

Arguably we could move notifications from InvokeBase to just ApplicationExecution because atm neon-js (which is used internally when doing an invoke) doesn't return the notifications for testinvoke and testscript calls. However, I'd rather update neon-js and have this already in the right place instead.

@ixje
ixje requested review from hotequil and raulduartep October 6, 2025 15:29
@hotequil
hotequil requested a review from luc10921 October 6, 2025 17:39

@hotequil hotequil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, @ixje! Ok for me. But @luc10921 will review this PR.

@luc10921 luc10921 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, there is a pre-commit hook that warns about formatting problems and cancels the commit if there is something wrong.
It should be triggering in the commits here.
If it's not sending the error when committing, try following these steps:

// Use a node version that is expected from this project
// ">=14.15.0 <15.0.0 || >=16.13.0 <17.0.0 || >=18.15.0 <=20.14.0",
nvm use 2.14.0
// I'm using nvm to be able to easily change node versions when needed
// https://github.com/nvm-sh/nvm

// Install rush.js https://rushjs.io/pages/intro/get_started/
npm install -g @microsoft/rush

With those steps, you should get an error message when trying to commit.

Checking formatting...
[warn] packages/neon-dappkit-types/src/Neo3EventListener.ts
[warn] packages/neon-dappkit-types/src/Neo3Invoker.ts
[warn] packages/neon-dappkit/src/NeonEventListener.ts
[warn] packages/neon-dappkit/test/NeonEventListener.spec.ts
[warn] Code style issues found in 4 files. Run Prettier to fix.

The script failed with exit code 1

You can also run the command rush lint to get the same error message without committing.

To fix the problems, run rush format.


export interface InvokeBase <T extends RpcResponseStackItem = RpcResponseStackItem> {
/** State of VM on exit. HALT means a successful exit while FAULT means exit with error. */
state: 'HALT' | 'FAULT'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why state instead of vmstate? The rpc method returns vmstate, it might be confusing for the user that checked the rpc documentation and is expecting it
https://docs.neo.org/docs/n3/reference/rpc/getapplicationlog.html

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't change the name. The original name on InvokeResult was state I just moved it to a common shared class. This way there are fewer changes needed. I personally think vmstate is nicer but if we go that route we should probably rename a lot more (particularly classes)

"changes": [
{
"packageName": "@cityofzion/neon-dappkit",
"comment": "upate to use latest type changes",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

update instead of upate

@ixje

ixje commented Oct 6, 2025

Copy link
Copy Markdown
Member Author

I didn't use rush format, but pretty sure i ran the format and lint command from the package.json files. I'll try the rush way if that matters.

Sounds like a good check to add to CI

@ixje ixje mentioned this pull request Oct 6, 2025
@ixje
ixje requested a review from luc10921 October 6, 2025 19:07
@luc10921
luc10921 merged commit ed67b2b into main Oct 6, 2025
1 check passed
@ixje
ixje deleted the streamline-eventlistener branch October 7, 2025 07: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.

4 participants