Skip to content

core: refactor loading orders from db#566

Merged
chappjc merged 4 commits into
decred:masterfrom
buck54321:matchlife
Jul 29, 2020
Merged

core: refactor loading orders from db#566
chappjc merged 4 commits into
decred:masterfrom
buck54321:matchlife

Conversation

@buck54321
Copy link
Copy Markdown
Member

For comparison with #564.

@buck54321
Copy link
Copy Markdown
Member Author

I think the advantage of this way is that there are no DB calls required when seeking detailed info on the order, such as will be needed with a solution to #559.

@chappjc
Copy link
Copy Markdown
Member

chappjc commented Jul 22, 2020

I thought about going this route. Do you think it's best to load all the inactive matches for other reasons? I can see how it would be helpful for #559 and other reasons. I'd be happy to go with this solution, but I would still like to consider changing the fill computation to be incremental, using AddFill. Regardless, that's not critical to this fix, so I don't see a need to change this PR.

@buck54321 buck54321 changed the title core: load all matches from database for active orders core: refactor loading orders from db Jul 29, 2020
@buck54321 buck54321 marked this pull request as ready for review July 29, 2020 12:22
Copy link
Copy Markdown
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retesting now, but looks good. Just a few thoughts.

Comment thread client/db/bolt/db.go Outdated
Comment thread dex/order/match.go Outdated
Comment thread client/db/types.go
ChangeCoin order.CoinID
// LinkedOrder is used to specify the cancellation order for a trade, or
// vice-versa.
LinkedOrder order.OrderID
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts about this being a []byte instead, which would save space in the DB and turn the zero ID checks into zero length or nil checks, but would require copying into and OrderID to retrieve the metaCancel in dbTrackers?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just storing it as nil if it's zeroed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, if that works out cleanly.

Comment thread client/db/bolt/db.go Outdated
}

var linkedID order.OrderID
copy(linkedID[:], getCopy(oBkt, linkedKey))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that now that we are storing nil, this Get will return []byte{} instead of nil. That's OK since there are no bytes to copy and it stays the zero OrderID. Just wanted to point that distinction out. I think that's what you envisioned for the decode now, right?

@chappjc chappjc merged commit 13c8959 into decred:master Jul 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants