-
Notifications
You must be signed in to change notification settings - Fork 254
feat(portfolio-contract): planner can reject promise/vow for plan #12183
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: aider (anthropic/claude-sonnet-4-20250514) <[email protected]>
This comment was marked as off-topic.
This comment was marked as off-topic.
turadg
left a comment
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.
I left some generic feedback. I'm not familiar enough with the issue to know whether this solves it. I think the other reviews can approve/deny but if you need me to let's talk so I can figure out how to prioritize it.
| portfolioId: number, | ||
| flowId: number, |
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.
somewhat tangential but is the nomenclature that flow/portfolio "ID" is a number and the string form (e.g. portfolio3) is a key?
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.
I don't think I've done that consistently.
I sort of think of the id being a number, and sometimes it's passed around as a js number and sometimes it's wrapped in a portfolioN form.
| return getCapDataStructure(storage.getValues(path).at(-1)); | ||
| }; | ||
|
|
||
| const getDeserialized = (path: string): unknown[] => { |
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.
why didn't storage.getDeserialized suffice? seems like a lower level concern than portfolio test supports per se
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.
I think it didn't exist somehow.
refs:
Description
Add a rejection API to the contract/planner interface.
Security / Scaling / Documentation Considerations
unremarkable
Testing Considerations
{ state: 'fail' }Upgrade Considerations
compatible behavior change only. no exo state changes. compatible with
ymaxControl.upgrade(...)