-
Notifications
You must be signed in to change notification settings - Fork 103
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
feat(x/ecocredit): add buy/sell expiration #671
Conversation
Codecov Report
@@ Coverage Diff @@
## master #671 +/- ##
==========================================
+ Coverage 75.88% 76.10% +0.21%
==========================================
Files 113 115 +2
Lines 18568 18834 +266
==========================================
+ Hits 14090 14333 +243
- Misses 3574 3587 +13
- Partials 904 914 +10
Flags with carried forward coverage won't be shown. Click here to find out more. |
pending #647 |
7d25b66
to
8a1bfd2
Compare
@@ -406,7 +406,7 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest | |||
// NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC) | |||
app.mm.SetOrderBeginBlockers( | |||
upgradetypes.ModuleName, capabilitytypes.ModuleName, minttypes.ModuleName, distrtypes.ModuleName, slashingtypes.ModuleName, | |||
evidencetypes.ModuleName, stakingtypes.ModuleName, ibchost.ModuleName, | |||
evidencetypes.ModuleName, stakingtypes.ModuleName, ibchost.ModuleName, /* ecocredit.ModuleName, */ |
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.
Any ideas why adding this would cause the tests to hang?
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.
It times out after 10 minutes with the following error:
tx.go:102:
Error Trace: tx.go:102
suite.go:118
tx.go:50
modules_test.go:12
Error: Received unexpected error:
rpc error: code = NotFound desc = rpc error: code = NotFound desc = account regen1huayfhrzpkxxws60dxlzv6p3l6e95nau7yk3as not found: key not found
Test: TestEcocreditIntegration
panic: test timed out after 10m0s
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.
i'm wondering if this has anything to do with the ecocredit module not fulfilling the interface that actually contains the BeginBlock signature.
might need to : add this to the ecocredit/module/module.go file var _ module.AppModule = Module{}
and implement the methods it says its missing. maybe that would work?
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.
I think it might be best to break this out into a separate issue. The ecocredit module does not implement the AppModule
interface and in attempting to do so there are some conflicts with the RegisterServices
method which uses a different Configurator
. Opened #689.
@@ -406,7 +406,7 @@ func NewRegenApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest | |||
// NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC) | |||
app.mm.SetOrderBeginBlockers( | |||
upgradetypes.ModuleName, capabilitytypes.ModuleName, minttypes.ModuleName, distrtypes.ModuleName, slashingtypes.ModuleName, | |||
evidencetypes.ModuleName, stakingtypes.ModuleName, ibchost.ModuleName, | |||
evidencetypes.ModuleName, stakingtypes.ModuleName, ibchost.ModuleName, /* ecocredit.ModuleName, */ |
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.
is ecocredit commented out due to errors?
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.
correct, not sure how to resolve at the moment: #671 (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.
let's leave it for a next task.
|
||
// new_expiration is the optional timestamp when the sell order expires. When the | ||
// expiration time is reached, the sell order is removed from state. | ||
google.protobuf.Timestamp new_expiration = 7 [ (gogoproto.stdtime) = 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.
do we really want to use new_
prefix?
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.
We are using new_quantity
and new_ask_price
so new_expiration
made sense.
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.
yeah, we don't do that in general. The other parts were not yet released. Let's chat about naming in the discord or standup.
return err | ||
} | ||
|
||
if !m.Orders[i].AskPrice.Amount.IsPositive() { | ||
if !order.AskPrice.Amount.IsPositive() { |
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.
IMHO, ValidateBasic
should be minimalistic. All other validation should go in other function: cosmos/cosmos-sdk#10680
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.
It looks like this is still in discussion. Maybe we can address this in all modules and all implementations of validate basic at a later point in time once there is an agreed upon solution?
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.
OK
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.
I think the stricter version is fine for now and that was more or less the outcome of the github discussion
x/ecocredit/server/expiration.go
Outdated
} | ||
|
||
var sellOrders []*ecocredit.SellOrder | ||
_, err = orm.ReadAll(sellOrdersIter, &sellOrders) |
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.
we only need to read the id, not the whole object.
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.
True but I'm not sure we have a way to only read the id. As far as I can see, we have to load the whole object here but maybe I'm missing something?
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.
Switched to LoadNext
to consolidate within the loop as suggested but still loading the whole object. Let me know if I'm missing something that would allow reading the id only.
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.
LoadNext
returns RowID
. I think this is the ID you are looking for. So we can save empty bytes as the value.
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.
Correct but LoadNext
still loads the whole object. We have to pass a proto message as the destination. RowID
returns the id in bytes, which we can convert, but we already have the id from the loaded object.
}, uint64(0)) | ||
if err != nil { | ||
panic(err.Error()) | ||
} |
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.
Other way to avoid the round trip with de-serialization of index, would be to add a delete method to the index (we can make a task for that). CC: @aaronc
context: #671 (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.
I've updated the time parsing flow and organized the tests - they are more readable (and shorter).
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.
thanks for the work! we can handle the issue with the index removal roundtrip in a separate task.
Description
Closes: #636
Add expiration to buy and sell orders and remove expired buy and sell orders with
BeginBlocker
.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change