-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add Auction Types #1726
Add Auction Types #1726
Conversation
Adds `BidTx` and friends to `v0_3`. The intention is to provide the necessary elements for first round of Marketplace integration. Since this is all hidden behind a version gate, our priority should be to satisfy integration necessities
We don't need it yet
Also some obsolete TODOs
types/src/v0/impls/auction.rs
Outdated
} | ||
|
||
/// Sign `BidTxBody` and return the signature. | ||
pub fn sign(&self, key: &EthKeyPair) -> Result<Signature, SigningError> { |
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 expect to use this function outside of this crate? If not, can it be a private function, or removed (i.e., signed
would call sign_builder_message
directly), to make it clearer that to get BidTx
we just need to use signed
?
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.
Yea, I guess my preference is in early integration is to have too much rather than not enough. df255e6
types/src/v0/impls/auction.rs
Outdated
let signature = FeeAccount::sign_builder_message(&key, body.commit().as_ref()).unwrap(); | ||
Self { signature, body } |
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.
Nit: Can we replace these lines with the signed
function?
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.
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.
May need a second approval in case I'm not caught up with everything yet, but this looks good to me! I mainly focused on reviewing the bid tx part.
I'm going to go ahead and merge b/c eve
I'm going to go ahead and merge b/c everything is gated behind a version anyway. |
This PR:
Adds
BidTx
and friends tov0_3
. The intention is to provide the necessary elements for first round of Marketplace integration. Since this is all hidden behind a version gate, our priority should be to satisfy integration necessities.This PR does not:
Attempt to implement many changes necessary for sequencer
v0_3
(alias Marketplace 0). We only need the basic types for now, and maybe some constructors and such.Key places to review:
types/src/v0/v0_3/auction.rs
types/src/v0/impls/auction.rs
is also included to provide constructors, but most of that logic will not be executed at this time.