-
Notifications
You must be signed in to change notification settings - Fork 211
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
Replace hardfork_at
with eras
object, and add node_era
#2468
Conversation
hardfork_at
with eras object
hardfork_at
with eras objecthardfork_at
with eras
object
@@ -2190,6 +2190,11 @@ taggedSumTypeOptions base opts = base | |||
{ sumEncoding = TaggedObject (_tagFieldName opts) (_contentsFieldName opts) | |||
} | |||
|
|||
explicitNothingRecordTypeOptions :: Aeson.Options | |||
explicitNothingRecordTypeOptions = defaultRecordTypeOptions | |||
{ omitNothingFields = False |
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.
Not sure why this is enabled by default…
"Is the mary
key not present because the wallet is not supporting mary
, or because the mary
fork has not yet occurred?"
Explicit null
for the latter case seems much clearer.
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 agree that the absence of a field doesn't have the same meaning as a null
value. I think that in the context of our API indeed, we most likely always want fields corresponding to Maybe values to be present 🤔
@@ -51,7 +51,8 @@ spec = describe "SHELLEY_NETWORK" $ do | |||
[ expectField #decentralizationLevel (`shouldBe` d) | |||
, expectField #desiredPoolNumber (`shouldBe` nOpt) | |||
, expectField #minimumUtxoValue (`shouldBe` Quantity minUTxOValue) | |||
, expectField #hardforkAt (`shouldNotBe` Nothing) | |||
, expectField (#eras . #shelley) (`shouldNotBe` Nothing) | |||
, expectField (#eras . #byron) (`shouldNotBe` Nothing) |
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.
Integration tests can in practice be run on any shelley-based era. So simply testing that at least byron
and shelley
are not null, should be pretty good.
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.
but then you will not receive information about mary and allegra, right? 🤔
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, fine, we can probably make the expectation dependent on the LOCAL_CLUSTER_ERA
value, by storing it in the test Context
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.
Hm, seems unexpectedly tedious though… (Can't use a shelley type in cardano-wallet-core-integration) Will look more at it later.
If and when each era started or will start. | ||
|
||
The object is keyed by era names. The values either describe the epoch boundary | ||
when the era starts (can be in the future or in the past), or are null if not yet | ||
confirmed on-chain. |
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.
Because of
(can be in the future or in the past)
wouldn't it make sense to provide a era
field either here, or in /network/information
?
A non-null mary
does not necessarily mean that we're in Mary. We could also be just before Mary.
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 thought it would go like this : we know something like mary will happen (null), then we know when it happens (by specifying era it starts), and we also now current slot/era. So everything is fine;-)
@@ -749,15 +751,30 @@ data ApiNetworkParameters = ApiNetworkParameters | |||
, decentralizationLevel :: !(Quantity "percent" Percentage) | |||
, desiredPoolNumber :: !Word16 | |||
, minimumUtxoValue :: !(Quantity "lovelace" Natural) | |||
, hardforkAt :: !(Maybe ApiEpochInfo) | |||
, eras :: !ApiEraInfo |
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.
why not to have
eras: [ApiEraInfo]
...
data Eras = Byron | Shelley | Allegra | Mary deriving (Eq, Ord, Show)
data ApiEraInfo = ApiEraInfo {
hardForkToEra :: !Era
hardFromEra :: !(Maybe Era)
timeOfTransition:: !(Maybe ApiEpochInfo)
}
?
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 is more robust in a sense that when new hard forks arrive they will just add new element to list. But maybe there is some rationale behind this way of storing era transitions?
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.
@KtorZ advocated for an object keyed by era-name since Daedalus wants to do stuff like isAllegra
and isShelley
, and already have to be highly aware of e.g. the ordering of them.
Although with #2468 (comment) in mind, I wonder if this changes 🤔
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.
Seems we should indeed add some kind of currentEra
field somewhere.
Maybe we should make this a list then, yes 🤔
Off the top of my head I would probably just do
data ApiEraInfo = ApiEraInfo {
eraName :: !Era
start :: !(Maybe ApiEpochInfo)
}
since hardFromEra
doesn't provide any extra information.
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.
In-doubt I would stick to the current object format. I think it still has its qualities too. If you feel strongly about it, feel free to disagree.
But I imagine, if a need arises where a list would be great for Daedalus, we can change it then. Currently the need for this is unclear.
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.
you got me wrong Johannes. I like your proposal, just was thinking loudly and sharing some ideas ;-) no pun intended.great job!
-- It might be cumbersome to work with this type. /But/ we don't need to. A | ||
-- product of @Maybe@ is both what we can query from the node, and | ||
-- what we need to provide in the wallet API. | ||
data EraInfo info = EraInfo |
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.
If you opt for my suggestion of ApiEraInfo then this part should also comply
_ -> | ||
LSQry $ QueryAnytimeMary GetEraStart | ||
eraBounds <- W.EraInfo | ||
<$> LSQry (QueryAnytimeByron GetEraStart) |
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.
👍
<<: *epochInfo | ||
nullable: True | ||
|
||
ApiEraInfo: &ApiEraInfo |
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.
the same here in case ApiEraInfo is refactored
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.
Sounds good, provided that we make sure of the behavior on start-up with regards to protocol parameters. I'd expect the server to block on a particular request until protocol parameters become available.
@@ -51,7 +51,8 @@ spec = describe "SHELLEY_NETWORK" $ do | |||
[ expectField #decentralizationLevel (`shouldBe` d) | |||
, expectField #desiredPoolNumber (`shouldBe` nOpt) | |||
, expectField #minimumUtxoValue (`shouldBe` Quantity minUTxOValue) | |||
, expectField #hardforkAt (`shouldNotBe` Nothing) | |||
, expectField (#eras . #shelley) (`shouldNotBe` Nothing) | |||
, expectField (#eras . #byron) (`shouldNotBe` Nothing) |
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.
👍
@@ -2190,6 +2190,11 @@ taggedSumTypeOptions base opts = base | |||
{ sumEncoding = TaggedObject (_tagFieldName opts) (_contentsFieldName opts) | |||
} | |||
|
|||
explicitNothingRecordTypeOptions :: Aeson.Options | |||
explicitNothingRecordTypeOptions = defaultRecordTypeOptions | |||
{ omitNothingFields = False |
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 agree that the absence of a field doesn't have the same meaning as a null
value. I think that in the context of our API indeed, we most likely always want fields corresponding to Maybe values to be present 🤔
"epoch_start_time": "1891-03-26T08:00:00Z", | ||
"epoch_number": 53 | ||
}, | ||
"byron": null, |
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.
That would be problematic in practice :)
ae508e2
to
c7c5b7a
Compare
47c5ddc
to
75c4f4e
Compare
75c4f4e
to
5f18e58
Compare
bors r+ |
2468: Replace `hardfork_at` with `eras` object r=Anviking a=Anviking # Issue Number ADP-676 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Replace `hardfork_at` with info of all eras - [x] Instead of storing ProtocolParameters in DB, just keep them in a `TVar` - [ ] <s>See if we can order the eras correctly in the `era` object (for when the JSON is viewed by humans)</s> Seems impossible to do. - [x] TODO: Extend swagger - [ ] Tiny doc tweaks # Comments - Could add absolute `slot` field also - Arguably a bit sad that the eras get sorted seemingly alphabetically in the json ```json $ cardano-wallet network parameters { "slot_length": { "quantity": 1, "unit": "second" }, "decentralization_level": { "quantity": 74, "unit": "percent" }, "genesis_block_hash": "5f20df933584822601f9e3f8c024eb5eb252fe8cefb24d1317dc3d432e940ebb", "blockchain_start_time": "2017-09-23T21:44:51Z", "desired_pool_number": 500, "epoch_length": { "quantity": 432000, "unit": "slot" }, "eras": { "shelley": { "epoch_start_time": "2020-07-29T21:44:51Z", "epoch_number": 208 }, "mary": null, "byron": { "epoch_start_time": "2017-09-23T21:44:51Z", "epoch_number": 0 }, "allegra": { "epoch_start_time": "2020-12-16T21:44:51Z", "epoch_number": 236 } }, "active_slot_coefficient": { "quantity": 5, "unit": "percent" }, "security_parameter": { "quantity": 2160, "unit": "block" }, "minimum_utxo_value": { "quantity": 1000000, "unit": "lovelace" } } ``` <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
hardfork_at
with eras
objecthardfork_at
with eras
object, and add node_era
Build failed: #expected |
We never want to use the old genesis parameters just because the wallet just started. The onTipUpdate will be run immediately on startup, and will fetch both interpreter, and protocol parameters. To be safe, and for readability, let's replace the TVar+initial val with an empty TMVar though.
5f18e58
to
f448ab7
Compare
bors r+ |
2468: Replace `hardfork_at` with `eras` object, and add node_era r=Anviking a=Anviking # Issue Number ADP-676 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Replace `hardfork_at` with info of all eras - [x] Instead of storing ProtocolParameters in DB, just keep them in a `TVar` - [ ] <s>See if we can order the eras correctly in the `era` object (for when the JSON is viewed by humans)</s> Seems impossible to do. - [x] TODO: Extend swagger - [ ] Tiny doc tweaks # Comments - Could add absolute `slot` field also - Arguably a bit sad that the eras get sorted seemingly alphabetically in the json ```json $ cardano-wallet network parameters { "slot_length": { "quantity": 1, "unit": "second" }, "decentralization_level": { "quantity": 74, "unit": "percent" }, "genesis_block_hash": "5f20df933584822601f9e3f8c024eb5eb252fe8cefb24d1317dc3d432e940ebb", "blockchain_start_time": "2017-09-23T21:44:51Z", "desired_pool_number": 500, "epoch_length": { "quantity": 432000, "unit": "slot" }, "eras": { "shelley": { "epoch_start_time": "2020-07-29T21:44:51Z", "epoch_number": 208 }, "mary": null, "byron": { "epoch_start_time": "2017-09-23T21:44:51Z", "epoch_number": 0 }, "allegra": { "epoch_start_time": "2020-12-16T21:44:51Z", "epoch_number": 236 } }, "active_slot_coefficient": { "quantity": 5, "unit": "percent" }, "security_parameter": { "quantity": 2160, "unit": "block" }, "minimum_utxo_value": { "quantity": 1000000, "unit": "lovelace" } } ``` <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Build failed:
Probably related to |
bors r+ |
2468: Replace `hardfork_at` with `eras` object, and add node_era r=Anviking a=Anviking # Issue Number ADP-676 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] Replace `hardfork_at` with info of all eras - [x] Instead of storing ProtocolParameters in DB, just keep them in a `TVar` - [ ] <s>See if we can order the eras correctly in the `era` object (for when the JSON is viewed by humans)</s> Seems impossible to do. - [x] TODO: Extend swagger - [ ] Tiny doc tweaks # Comments - Could add absolute `slot` field also - Arguably a bit sad that the eras get sorted seemingly alphabetically in the json ```json $ cardano-wallet network parameters { "slot_length": { "quantity": 1, "unit": "second" }, "decentralization_level": { "quantity": 74, "unit": "percent" }, "genesis_block_hash": "5f20df933584822601f9e3f8c024eb5eb252fe8cefb24d1317dc3d432e940ebb", "blockchain_start_time": "2017-09-23T21:44:51Z", "desired_pool_number": 500, "epoch_length": { "quantity": 432000, "unit": "slot" }, "eras": { "shelley": { "epoch_start_time": "2020-07-29T21:44:51Z", "epoch_number": 208 }, "mary": null, "byron": { "epoch_start_time": "2017-09-23T21:44:51Z", "epoch_number": 0 }, "allegra": { "epoch_start_time": "2020-12-16T21:44:51Z", "epoch_number": 236 } }, "active_slot_coefficient": { "quantity": 5, "unit": "percent" }, "security_parameter": { "quantity": 2160, "unit": "block" }, "minimum_utxo_value": { "quantity": 1000000, "unit": "lovelace" } } ``` <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Jira will detect and link to this PR once created, but you can also link this PR in the description of the corresponding ticket ✓ Acknowledge any changes required to the Wiki ✓ Finally, in the PR description delete any empty sections and all text commented in <!--, so that this text does not appear in merge commit messages. --> Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Build failed:
Possibly: |
bors r+ |
Build succeeded: |
Issue Number
ADP-676, ADP-681
Overview
Instead of storing ProtocolParameters in DB, just keep them in a
TVar
Remove problem where the wallet might use genesis-derived protocol parameters and era on wallet start up, until the node first changed tip.
Replace
hardfork_at
with info of all erasAdd new
node_era
field in GET /network/informationSee if we can order the eras correctly in theSeems impossible to do.era
object (for when the JSON is viewed by humans)Comments
slot
field also