[Utility] Foundational bugs, tests, code cleanup and improvements (2/3) #550

Olshansk · 2023-02-27T01:21:29Z

Description

The second of three changes necessary to refactor and improve the utility module to enable implementation of future, more complex, interfaces.

While the list of changes in #503 was more bug & testing focused, this PR was more readability & documentation focused in preparation for M3.

Issue

Fixes #504

Type of change

Please mark the relevant option(s):

List of changes

Fixed bug where we were not removing txs from the mempool of replicas
Improved the readability of the Block lifecycle and Message validation
Replace unnecessary functions in such as Store(), getStoreAndHeight() and a few otherwise
Moved the Transaction proto to the shared module
Added documentation to TxResult result and removed DefaultTxResult and
Added extensive documentation to functions involved in the Block lifecycle
Removed unnecessary fields from the BlockHeader
Renamed GenericParam to ServiceURL
Consolidated StakeStatus in the shared package
Renamed governance parametesr related to rate limitting Applications
Renamed applyTx to hydrateTx and added documentation on its functionality

Testing

make develop_test
LocalNet w/ all of the steps outlined in the README

Required Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have tested my changes using the available tooling
I have updated the corresponding CHANGELOG

If Applicable Checklist

I have updated the corresponding README(s); local and/or global
I have added tests that prove my fix is effective or that my feature works
I have added, or updated, mermaid.js diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

…owner and a few other small things

deblasis

Yo @Olshansk! I gave this a first pass. Solid work as usual.

Some bits and bolts and Qs. Depending on the priority of this vs what I am working on currently let me know if and how I can help you here. 🫡

Co-authored-by: Alessandro De Blasis <alex@deblasis.net>

Olshansk

@deblasis Thank you for the quick turnaround on the review.

PTAL at the changes I made along with the comments I left on the unresolved comments.

I think you have a good sense of some future follow-up work from the changes + the couple of issues I created out of it.

I want to prioritize getting this in given the size of the PR and because I'll be OOO later this week so there won't be much time to maintain & rebase it.

Olshansk · 2023-02-27T23:29:03Z

runtime/test_artifacts/generator.go

-			genericParam = DefaultMaxRelaysString
-		}
-		actor, pk := NewDefaultActor(int32(actorType), genericParam)
+		serviceUrl := getServiceURL(i + 1)


I changed everything I could to URL. The only thing thing that remains is the Url that's autogenerated by proto-c-gen which unfortunately can't be changed...

I added a comment to actor.proto:

// proto-gen-c does not support `go_name` at the time of writing resulting // in the output go field being snakeCase: ServiceUrl (https://github.com/golang/protobuf/issues/555)

runtime/test_artifacts/generator.go

Olshansk · 2023-02-27T23:35:18Z

runtime/genesis/proto/genesis.proto

 message Params {
+  reserved 4, 59; // Deprecated


One of the reasons I hesitated was to avoid doing it manually, but then I discovered: https://marketplace.visualstudio.com/items?itemName=ripwu.protobuf-helper

Ty for the push

Olshansk · 2023-02-27T23:50:16Z

shared/core/types/proto/actor.proto

+  repeated string chains = 4; // Not applicable to `Validator` actors
+  string service_url = 5; // Not applicable to `Application` actors


// TODO(#555): Investigate ways of having actor specific params that are not part of the shared struct. message Actor {

Olshansk · 2023-02-28T00:04:42Z

shared/core/types/transaction.go

 	// Nonce cannot be empty to avoid transaction replays
 	if tx.Nonce == "" {
-		return ErrEmptyNonce()
+		return fmt.Errorf("nonce cannot be empty")


shared/core/types/transaction_test.go

utility/transaction.go

utility/validator.go

deblasis

🚢 🇮🇹

Olshansk · 2023-02-28T21:41:50Z

@deblasis Thank you again for the quick turnaround. I know this was a bit rushed but I did a local rerun of the unit tests and LocalNet (with docker-compose + sending a transaction) and verified that it works.

🚢 🇮🇹

I'm on season 2 of the White Lotus, so I guess I'll see you in Sicily.

deblasis · 2023-02-28T22:16:29Z

@deblasis Thank you again for the quick turnaround. I know this was a bit rushed but I did a local rerun of the unit tests and LocalNet (with docker-compose + sending a transaction) and verified that it works.

👍💯

🚢 🇮🇹

I'm on season 2 of the White Lotus, so I guess I'll see you in Sicily.

Never watched, no idea what's about but I hear it's pop culture already.

LOL. Funny. I might be going to Sicily for real in a few weeks...

* main: [Utility] Foundational bugs, tests, code cleanup and improvements (2/3) (#550) [CONSENSUS] Find issue with sending metadata request (#548) [Tooling] SLIP-0010 HD Child Key Generation (#510)

* main: [Libp2p] Add libp2p module directories and helpers (part 1) (#534) [P2P, Runtime] Update P2P & base config (part 2) (#535) [Utility] Foundational bugs, tests, code cleanup and improvements (2/3) (#550) [CONSENSUS] Find issue with sending metadata request (#548) [Tooling] SLIP-0010 HD Child Key Generation (#510)

jessicadaugherty · 2023-03-09T17:32:56Z

Part 3/3 can be found here: #574

Olshansk added 28 commits February 21, 2023 14:00

Rename converters to utils

4b9593f

s/app_staking_adjustment_owner/app_stake_to_session_quota_multiplier_…

85866b7

…owner and a few other small things

Renamed the rest of the staking adjustments

750c009

Consolidated stake status

1a1187c

Consolidated stake status proto

4e62d37

All tests passing again

0875fef

Removed mockBusInTestModules

69859b6

Removed the whole getStoreAndHeight thing

ec2e0b2

Merge with main

e17772b

Resolved merge conflicts with master

52066cf

s/quota/tokens

244963d

Replace GenericParam with ServiceUrl

4bfa3b9

Removed maxRelays

acfdfc6

Removed baseline_stake_rate

ecca9cd

Tests passing again

0386df2

Moved transaction.proto from utility/types to shared/core/types

0da331a

Removed DefaultTxResult

28bb630

Merge with main

e56bcde

Added a comment

b912033

Applying an old stashed commit

da065bc

Reviewed validator.go

769ded3

Re-reviewed the utility module interface

47182e9

Mostly reviewed block.go but still in progress

519f762

Reviewed session.go

d6f1b0d

Remove consensus/types/proto/validator.proto

21057a9

Reviewed tx_result.go

090a53d

A few minor changes to block.go

d0d005d

fix tests in tx_result.go

395de66

Olshansk added the utility Utility specific changes label Feb 27, 2023

Olshansk added this to the M3: Pocket RoS (Relay or Slash) milestone Feb 27, 2023

deblasis suggested changes Feb 27, 2023

View reviewed changes

Olshansk and others added 3 commits February 27, 2023 14:19

Apply suggestions from deblasis

b1758e0

Co-authored-by: Alessandro De Blasis <alex@deblasis.net>

Renamed Url to URL

f23f479

Rename keygenerator package to keygen

7c4fbce

Olshansk mentioned this pull request Feb 27, 2023

[Core] Redesign actor.proto to handle actor specific parameters #555

Open

10 tasks

Added comments to actor.proto

4f8f24c

Olshansk mentioned this pull request Feb 28, 2023

[Utility] Leverage Pocket Errors in the entire codebase #556

Open

8 tasks

Olshansk and others added 9 commits February 27, 2023 16:05

Add comments related to 556

db69bc8

Moved TxGossipMessageContentType from utility to shared

183983f

Updated more comments

53219ad

Minor edits

086e7a7

Update utility/block.go

51939b3

Co-authored-by: Alessandro De Blasis <alex@deblasis.net>

Update changelogs

bd1d89a

Pop Tx from mempool when applying block

0538925

Fixing remaining lingers

47d1d92

Fix broken test

eda1999

Olshansk requested a review from deblasis February 28, 2023 01:00

Olshansk commented Feb 28, 2023

View reviewed changes

Fix utility changelog

007e2bb

deblasis approved these changes Feb 28, 2023

View reviewed changes

Merge with main

2dbcb7f

Olshansk merged commit 56ea777 into main Feb 28, 2023

Olshansk deleted the issues/504/utility_2_2 branch February 28, 2023 21:42

Olshansk mentioned this pull request Mar 8, 2023

[TECHDEBT] [Persistence] Fix Persistence Module TODOs - Golang Test Cleanup & Teardown #208

Closed

7 tasks

jessicadaugherty mentioned this pull request Mar 9, 2023

[WIP][Utility] Foundational bugs, tests, code clean up, and improvements (3/3) #574

Open

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Utility] Foundational bugs, tests, code cleanup and improvements (2/3) #550

[Utility] Foundational bugs, tests, code cleanup and improvements (2/3) #550

Olshansk commented Feb 27, 2023 •

edited

Loading

deblasis left a comment

Olshansk left a comment

Olshansk Feb 27, 2023

Olshansk Feb 27, 2023

Olshansk Feb 27, 2023

Olshansk Feb 28, 2023

deblasis left a comment

Olshansk commented Feb 28, 2023

deblasis commented Feb 28, 2023

jessicadaugherty commented Mar 9, 2023

		repeated string chains = 4; // Not applicable to `Validator` actors
		string service_url = 5; // Not applicable to `Application` actors

[Utility] Foundational bugs, tests, code cleanup and improvements (2/3) #550

[Utility] Foundational bugs, tests, code cleanup and improvements (2/3) #550

Conversation

Olshansk commented Feb 27, 2023 • edited Loading

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

deblasis left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk Feb 27, 2023

Choose a reason for hiding this comment

Olshansk Feb 27, 2023

Choose a reason for hiding this comment

Olshansk Feb 27, 2023

Choose a reason for hiding this comment

Olshansk Feb 28, 2023

Choose a reason for hiding this comment

deblasis left a comment

Choose a reason for hiding this comment

Olshansk commented Feb 28, 2023

deblasis commented Feb 28, 2023

jessicadaugherty commented Mar 9, 2023

Olshansk commented Feb 27, 2023 •

edited

Loading