-
Notifications
You must be signed in to change notification settings - Fork 0
Update to align with new comp cv rule #13
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
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.
Thanks for doing this, this does simplify our protos a lot. However, I think there are still lingering redundancies that could provide confusion down the road.
Chris = 2; | ||
Daniel = 3; | ||
enum AirdropType { | ||
Undefined = 0; |
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.
After some thought, I'd cut this --> I don't want to be checking for undefined everytime I use this type.
message Airdrop { | ||
AirdropIndex Index = 1; // Unique ID | ||
ODLCObjects Object = 2; // Unique Target | ||
AirdropType Index = 1; // Unique 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.
This seems redundant with AirdropType, now that they are single, unique objects.
*/ | ||
message AirdropSwap { | ||
AirdropIndex index = 1; | ||
AirdropType index = 1; |
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.
do we want to break stuff
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 is a mandatory change, or else protos just wont compile because we dont have airdropindex anymore.
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.
sorry, I mean the entire type looks redundant. My bad, do we want to break stuff refers to deleting this.
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.
sure, we can nuke this. ill just check the references for this from gcs and obc
a16cd9d
to
041a4b1
Compare
Closes #11
Don't merge until we're done with the corresponding obc and gcs changes