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

refactoring for encoding logic #309

Merged
merged 7 commits into from
Nov 13, 2023
Merged

refactoring for encoding logic #309

merged 7 commits into from
Nov 13, 2023

Conversation

rabbitprincess
Copy link
Member

What is changed?

small refactoring about encode ( hex, base58, base58check, base64, gob )
replace encode logic to using internal package

@rabbitprincess rabbitprincess added the check hardfork Run sync test with the PR label Nov 3, 2023
@kroggen
Copy link
Member

kroggen commented Nov 3, 2023

Why is the GobDecode in the common package instead of together with the others in enc?

Is there some requirement for that?

@kroggen
Copy link
Member

kroggen commented Nov 3, 2023

The conversion from enc.ToString to enc.B58Encode makes it clear what is the encoding format.

I am just not sure if the long name would not be better, like Base58 instead of B58


I would prefer to use names like this instead of enc:

base64.Encode
base58.Encode
base58check.Encode
hex.Encode

So all would have a consistent usage, with {format}.(Encode|Decode)

This would require to have each of these inside of "enc", like "enc/base64", "enc/hex"...

@kroggen
Copy link
Member

kroggen commented Nov 3, 2023

In types/common.go we can still find EncodeB64 and EncodeB58

@rabbitprincess
Copy link
Member Author

Why is the GobDecode in the common package instead of together with the others in enc?

Is there some requirement for that?

in before enc package was just b58 encode / decode. i`ll move gob function from common to enc.

@rabbitprincess
Copy link
Member Author

In types/common.go we can still find EncodeB64 and EncodeB58

internal package doesn`t reveal for other repository, but indexer / tx_replay should use encode/decode logic. so it is good to leave in types.

@kroggen
Copy link
Member

kroggen commented Nov 6, 2023

It appears OK to me

@hayarobi What do you think?

@rabbitprincess
Copy link
Member Author

rabbitprincess commented Nov 6, 2023

it need sync test @kslee8282

@kslee8282
Copy link
Member

it need sync test @kslee8282

Nov 6 06:07:57.931 INF ../go/aergo/cmd/aergosvr/aergosvr.go:100 > AERGO SVR STARTED branch=feature/encoding module=asvr revision=04c5a261
start sync mainnet

@kslee8282
Copy link
Member

sync success

@kslee8282 kslee8282 added the pass sync test The PR can be merged label Nov 13, 2023
@kroggen
Copy link
Member

kroggen commented Nov 13, 2023

Simple conflict resolved

@rabbitprincess rabbitprincess merged commit 1c0d59f into develop Nov 13, 2023
3 checks passed
@rabbitprincess rabbitprincess deleted the feature/encoding branch November 13, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
check hardfork Run sync test with the PR pass sync test The PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants