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

chore (pkg/scale): Integrate scale into lib/genesis #1663

Closed
wants to merge 45 commits into from

Conversation

jimjbrettj
Copy link
Contributor

Changes

  • Integrate new scale pkg into lib/genesis
  • Replace AccountInfo type and integrate scale pkg whereever it is used
  • Updates to encoding for Uint128

Tests

go test ./lib/genesis -v

go test ./lib/runtime/wasmer -v

go test ./dot/rpc/modules -v

Issues

@jimjbrettj jimjbrettj added the wip label Jun 29, 2021
@jimjbrettj jimjbrettj changed the title Jimmy/scale integrate genesis chore (pkg/scale): Integrate scale into lib/genesis Jun 29, 2021
@jimjbrettj jimjbrettj marked this pull request as draft June 29, 2021 21:28
@jimjbrettj jimjbrettj marked this pull request as ready for review June 29, 2021 22:26
@jimjbrettj jimjbrettj added scale and removed wip labels Jun 29, 2021
}
}
}
//
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this all commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was causing a circular dependency issue so I commented out for now and Tim said he would need to revise the package for this test

Comment on lines +354 to +357
if i == nil {
err = fmt.Errorf("uint128 is nil: %v", i)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

thought this would be handled higher up - @timwu20 is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might not be. I added because when I was initially encoding with the old AccountInfo type there were cases where the fields weren't initialized and it was crashing. Shouldn't be an issue now since I changed that but I kept this anyways. Can remove if not necessary

Copy link
Contributor

@timwu20 timwu20 Jun 30, 2021

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 should be necessary. In most use cases when encoding, you would've have initialised a value for that pointer, instead of a zero case pointer. Nevertheless, it shouldn't panic in the event a zero case pointer is passed in. Can you point me to the code that causes this panic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah dot/rpc/modules/system_test.go where the AccountInfo type was used the struct fields weren't initiated and thats what caused my initial issue. This pr changed it so now they are initialized now

* refactor telemetry messages to map format

* add basic network state telemetry message

* refactor message sender to handle interface{} types

* refactor telemetry messages to be structs

* lint

* go fmt

* lint

* move msg building logic outside msg sending loop

* make telemetry messages an interface

* Lookup transactions count from TransactionsState

* address comments

* fix mocks for tests

* lint

* refactor TelemetryMessage to Message

* update mock handler to return result

* add TransactionsCount to mockhandler

* move logic to build new network state message

* lint

* fix interface

* update mockhandler

* lint
Copy link
Contributor

@noot noot left a comment

Choose a reason for hiding this comment

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

not sure what happened but looks like there's a bunch of extra changes here, can you merge/rebase off development again? looks like something went wrong when merging

@jimjbrettj jimjbrettj closed this Jul 1, 2021
@jimjbrettj jimjbrettj reopened this Jul 1, 2021
@jimjbrettj jimjbrettj marked this pull request as draft July 1, 2021 19:30
@jimjbrettj jimjbrettj closed this Jul 1, 2021
@jimjbrettj jimjbrettj deleted the jimmy/scale-integrate-genesis branch July 5, 2021 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants