-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: mark as submitted only those bids that were actually submitted #367
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
@@ -415,7 +416,7 @@ impl AuctionManager<Svm> for Service<Svm> { | |||
&self, | |||
_permission_key: entities::PermissionKey<Svm>, | |||
bids: Vec<entities::Bid<Svm>>, | |||
) -> Result<entities::TxHash<Svm>> { | |||
) -> Result<Vec<Result<entities::TxHash<Svm>>>> { | |||
if bids.is_empty() { |
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.
can we simplify this function return type to Vec<Result<entities::TxHash<Svm>>>
and remove the len 0 check?
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.
mmmm I tried this and it's hard because in the EVM version we get a single error for the whole vector and returning a vector of results would requiring cloning that error which is not allowed.
I do agree about removing the 0-length check, it seems outside of the responsibility of this 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.
on second thoughts since all we do with the errors is logging... we can log them inside submit_bids
if you want I could do Result<Vec<Option>>
or Vec<Option>
.
Wdyt?
self.update_bid_status(UpdateBidStatusInput { | ||
new_status: Service::get_new_status( | ||
&bid, | ||
&submitted_bids, |
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.
With this change, we are marking the bids that are not submitted (failed to be submitted) on chain as lost, and we broadcast that their bid is lost (even when it's a winner bid and we were unable to submit it). Are we sure about this UX?
I fix this bug where some SVM bids were being marked as submitted despite
send_transaction
failing to submit them