-
Notifications
You must be signed in to change notification settings - Fork 31
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
Creation of consolidation transaction for stock #480
Creation of consolidation transaction for stock #480
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, @arthur-clara! Couple minor things:
- Can you merge the latest changes from main back in? I just merged a bunch of PRs and there are a few conflicts and a change to the doc generator which will change the headers in your docs in this PR.
- I'd suggest doing minItems: 2 and uniqueItems: True attrs on the Consolidation Transaction primitive.
"type": "string" | ||
} | ||
}, | ||
"resulting_security_id": { |
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.
@arthur-clara, can you use a minItems validation here to ensure there are two or more ids? See here for an example:
Open-Cap-Format-OCF/schema/primitives/objects/transactions/transfer/Transfer.schema.json
Lines 16 to 24 in 7abfda1
"resulting_security_ids": { | |
"title": "Security Transfer - Resulting Security ID Array", | |
"description": "Array of identifiers for new security (or securities) created as a result of the transaction", | |
"type": "array", | |
"items": { | |
"type": "string" | |
}, | |
"minItems": 1, | |
"uniqueItems": true |
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.
Actually, thinking about #471, @arthur-clara, maybe we can make this more general purpose? Like a multiple security transaction primitive which you can compose into your consolidation transaction. See more here - #471 (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.
This should be "MultipleSecurityTransaction" or something similar. Don't put in the resulting_security_ids in the primitive. Also, let's keep the Consolidation primitive even if it's empty as we've done elsewhere.
What type of PR is this?
/kind feature
This is the first cut of the addition of the stock consolidation. For review and discussion.
What this PR does / why we need it:
This PR creates a new Stock Transaction called "Consolidation".
See the discussion by VictorMimo on issue 449 on why it is needed.
Which issue(s) this PR fixes:
Fixes #449