-
Notifications
You must be signed in to change notification settings - Fork 45
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: return tx block height info (decoupled approach) #386
feat: return tx block height info (decoupled approach) #386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a couple of small comments inline.
As mentioned in the issue, I think this is a better approach as additional code complexity is pretty low and the downsides of doing this vs. putting transactions into the AccountSummary
are pretty small.
Oh - and let's update the readme docs and the changelog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Left a comment/question.
0d98a50
to
f232d23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left a few comments inline - mostly related to naming/docs.
Should we take this PR out of "draft" mode?
Also, let's update the changelog.
378167c
to
aa1a40b
Compare
…ifierUpdate contain
I'll close the other PR (#383) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good! Thank you!
Do we have an issue in miden-client
to make use of these changes?
addresses #380
This is an alternative approach to #383