Skip to content
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

wctl refactoring: Concrete data structures and code cleanup #319

Closed
wants to merge 155 commits into from

Conversation

diamondburned
Copy link
Contributor

@diamondburned diamondburned commented Nov 5, 2019

Changes:

  • Changes logging from key-value to concrete struct with proper unmarshaling

Known breaking changes:

  • num_accounts_in_store is removed, as num_accounts has the same value.

Other minor changes:

  • Websocket ignores all events below info
  • Logging methods are now very different (refer to ledger.go changes).
  • API package is refactored.
  • Events logged into the CLI are now scattered in wavelet and api packages. No more manually updating fields in wctl, cli and elsewhere.

This PR solves #422.

! This PR is untested !
The PR now passes tests.

Hasyimi Bahrudin and others added 30 commits October 15, 2019 21:18
* wavelet: remove gossip mechanism

* wavelet: fix dependencies
* all: remove transaction creator. replace it with transaction sender

* all: rename Sender Signature to Signature

* ledger, collapse, contract, tx: WIP add mempool and blocks. update collapseTransactions

* ledger, genesis, db, block: replace rounds with blocks

* block: make block implements snowball.Identifiable

* all: implement query, modify snowball, update protocol.
* wavelet: remove DownloadTx RPC and SyncTransactions method

* wavelet, conf: use bloom filter for detecting missing tx

* ledger: add existing txs in graph to bloom filter

* ledger: always add tx to bloom filter

* ledger: add delay between pulls
…e rounds data structure and rename any references from rounds to blocks

ledger: add todos for block proposal, and fix build errors for any logs
genesis, block, ledger: add todos for merkle root derivation, and derive initial root for the genesis block
sys, config, cmd/wavelet: rename SyncIfRoundsDifferBy to SyncIfBlockIndicesDifferBy, and RewardWithdrawalsRoundLimit to RewardWithdrawalsBlockLimit
# Conflicts:
#	block.go
#	common.go
#	conf/conf.go
#	genesis.go
#	ledger.go
#	protocol.go
#	vote.go
@diamondburned diamondburned changed the title Wctl cleanup wctl cleanup Nov 30, 2019
@diamondburned diamondburned marked this pull request as ready for review November 30, 2019 07:32
@diamondburned diamondburned changed the base branch from himitsu to master November 30, 2019 07:32
@diamondburned diamondburned self-assigned this Dec 12, 2019
@diamondburned diamondburned changed the title wctl cleanup [WIP] wctl cleanup Dec 12, 2019
@diamondburned diamondburned added enhancement New feature or request quality improvements This item indicates the need for or supplies changes that improve maintainability labels Dec 12, 2019
@a-urth

This comment has been minimized.

@diamondburned

This comment has been minimized.

@diamondburned diamondburned changed the title [WIP] wctl cleanup wctl refactoring: Concrete data structures and code cleanup Jan 12, 2020
Copy link
Contributor

@a-urth a-urth left a comment

Choose a reason for hiding this comment

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

TestPollLog should be investigated and fixed or changed according to other changes - since it was and still is stable in master.
Also, all linting errors should be resolved or muted if fixing is not possible

ev.Msg("Updated account " + updated + ".")
}

// yes, I know this is a terrible function name, it can't be helped
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this comment is necessary.

}

// yes, I know this is a terrible function name, it can't be helped
func unmarshalAccountIDandUint64(v *fastjson.Value, id AccountID, ukey string, u *uint64) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

it definitely should be renamed at least to something containing amount instead of uint64.
also maybe 2 separate functions might be used - one for account and another one for amount? even though there will be two function calls, intention and naming will be much cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request quality improvements This item indicates the need for or supplies changes that improve maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants