feat(marketplace): Buy ORM
#899
Conversation
| return err | ||
| } | ||
| supply.RetiredAmount = supplyRetired.String() | ||
| } |
There was a problem hiding this comment.
lot of repeated "decode + add". Would be good to create a helper function for that
There was a problem hiding this comment.
BTW, why did we decide to store numbers as string rather than bytes?
There was a problem hiding this comment.
not sure, just how its been done so far. perhaps we need an issue/discussion on it
There was a problem hiding this comment.
BTW, why did we decide to store numbers as string rather than bytes?
I thought we talked about this several times and everyone understood why. Since it's not clear can someone please make sure this is well documented so it doesn't keep coming up?
- we have no agreed upon standard for encoding big decimals or bid integers as bytes within the SDK, let alone in client applications
- decimal and integer types often need to be signed in transactions and need to be human readable for Ledger signing
- trying to find a solution for the above has always been seen as a premature optimization that will make debugging harder
at some point, a bytes standard may be useful, but for now I would consider it very low priority
There was a problem hiding this comment.
Yes, let's continue in an issue - it's the same discussion as we have for addresses. Just for note that ledger eventually will use different serialization anyway.
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
…en-ledger into ty/856-buy_orm
| if err != nil { | ||
| return nil, fmt.Errorf("sell order %d: %w", selection.SellOrderId, err) | ||
| } | ||
| if sellOrder.DisableAutoRetire != order.DisableAutoRetire { |
There was a problem hiding this comment.
They don't need to be equal. If order.DisableAutoRetire == true, then sellOrder.Disable must == true. Otherwise we don't care.
There was a problem hiding this comment.
There was a problem hiding this comment.
we should also make the error specific. it's not a mismatch, it's that we're trying to avoid retiring credits when they must be retired
| return err | ||
| } | ||
| supply.RetiredAmount = supplyRetired.String() | ||
| } |
There was a problem hiding this comment.
BTW, why did we decide to store numbers as string rather than bytes?
I thought we talked about this several times and everyone understood why. Since it's not clear can someone please make sure this is well documented so it doesn't keep coming up?
- we have no agreed upon standard for encoding big decimals or bid integers as bytes within the SDK, let alone in client applications
- decimal and integer types often need to be signed in transactions and need to be human readable for Ledger signing
- trying to find a solution for the above has always been seen as a premature optimization that will make debugging harder
at some point, a bytes standard may be useful, but for now I would consider it very low priority
| sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
| ) | ||
|
|
||
| func TestBuy_ValidTradable(t *testing.T) { |
There was a problem hiding this comment.
Would really like to see BDD tests... by the time I got to this part of the PR my attention span was like 😴
There was a problem hiding this comment.
Can you review #902 when you get a chance? I think we need to establish a standard for writing acceptance tests before we try implementing them throughout.
There was a problem hiding this comment.
That PR looks good to me. Any particular questions you have @ryanchristo ?
There was a problem hiding this comment.
Not at the moment but I'm sure I will have questions when migrating the existing acceptance tests (#895), especially on how we can reduce complexity and reuse initial setup for multiple test cases.
There was a problem hiding this comment.
I think we'll want to 1) follow best practices for writing Gherkin - which hopefully that book will help with and 2) come up with best practices for implementing in golang.
I notice you needed to use gomock AnyTimes... things like that might be hard to figure out each time and it would be good to document best practices + tips
Co-authored-by: Aaron Craelius <aaron@regen.network>
Description
ref: #856
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.mdReviewers 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