diamondhands rosetta review 2024-02-26 #69
Conversation
diamondhands0
force-pushed
the
diamondhands/ROSETTA-REVIEW-2024-02-26
branch
from
304c35e
to
b410d43
Compare
diamondhands0
force-pushed
the
diamondhands/ROSETTA-REVIEW-2024-02-26
branch
from
b410d43
to
475aa1a
Compare
| } | ||
| } |
There was a problem hiding this comment.
Nit: I think this would work just fine and be less "magic number"-ey if we replaced it with:
return byte(balanceType.BalanceTypePrefix()). So the Hypersync prefixes are actually just: {PrefixHypersyncBlockHeightToBalances, whatever the balance prefix is}.
Whole function would look like this:
func (balanceType BalanceType) HypersyncBalanceTypePrefix() byte {
return byte(balanceType.BalanceTypePrefix())
}
There was a problem hiding this comment.
Actually you could just delete this function and use byte(balanceType.BalanctTypePrefix()) in its place.
There was a problem hiding this comment.
No we can't use byte(balanceType.BalanceTypePrefix()) as this breaks backwards compatibility. We can use byte(balanceType) if you prefer.
| @@ -24,171 +24,238 @@ const ( | |||
|
|
|||
There was a problem hiding this comment.
Nit: Add a comment that PrefixUtxoOps is no longer used because we now pull UtxoOps from the node's BadgerDB.
| return prefix | ||
| } | ||
|
|
||
| func (index *RosettaIndex) PutHypersyncBlockBalances(blockHeight uint64, isLocked bool, balances map[lib.PublicKey]uint64) { | ||
|
|
||
| func (index *RosettaIndex) PutHypersyncBlockBalances( |
There was a problem hiding this comment.
As discussed, you'll need to change these Puts to take a WriteBatch or whatever but I'll assume we'll handle this in a follow-up PR.
| glog.Errorf("PutLockedStakeBalanceSnapshot: %v", err) | ||
| } | ||
|
|
||
| // TODO: Add support for locked DESO balance entries via coin lockups here. |
There was a problem hiding this comment.
Delete this comment as we no longer need to support this.
| @@ -563,6 +690,10 @@ func (node *Node) getSwapIdentityOps(txn *lib.MsgDeSoTxn, utxoOpsForTxn []*lib.U | |||
| }, | |||
| }) | |||
|
|
|||
| // TODO: Do we need to do this for ValidatorEntry and LockedStakeEntries as well? If so, | |||
| // we'll need to expose add ToValidatorEntry, FromValidatorEntry, ToLockedStakeEntries, and | |||
| // FromLockedStakeEntries to the UtxoOperation. | |||
There was a problem hiding this comment.
Oof this is annoying. I think unfortunately we need to swap the following things in the case of SwapIdentity:
- The stake amounts of the two validators that we're swapping, since validators are indexed by pkid
- All of the LockedBalanceEntries, since they're also indexed by pkid
Alternatively, we can disable SwapIdentity after the PoS cutover. I'll float this for discussion in Slack.
There was a problem hiding this comment.
This comment should have been deleted. ValidatorEntry and LockedStakeEntry are tracked by PKID so we don't need to do anything when a swap identity occurs.
| @@ -57,6 +59,7 @@ func accountBalanceCurrent(node *deso.Node, account *types.AccountIdentifier) (* | |||
| return nil, wrapErr(ErrDeSo, err) | |||
| } | |||
|
|
|||
| // TODO: Hook up new PosMempool | |||
There was a problem hiding this comment.
You don't need this comment anymore right?
No description provided.