Skip to content

Genesis address format - change address type from byte slice to string#173

Merged
abergasov merged 3 commits into
masterfrom
3315-genesis-address-format_api_signature_change
Aug 8, 2022
Merged

Genesis address format - change address type from byte slice to string#173
abergasov merged 3 commits into
masterfrom
3315-genesis-address-format_api_signature_change

Conversation

@abergasov
Copy link
Copy Markdown
Contributor

@abergasov abergasov commented Jul 12, 2022

what changed

changed all address fields from []byte to bech32 string smt1ru1241...

See spacemeshos/go-spacemesh#3315

Closes #172

@abergasov abergasov changed the title change address type from byte slice to string 3315 Genesis address format - change address type from byte slice to string Jul 13, 2022
@abergasov abergasov requested review from dshulyak and lrettig and removed request for dshulyak July 13, 2022 07:58
@lrettig
Copy link
Copy Markdown
Collaborator

lrettig commented Jul 13, 2022

Please check whether this also needs to change:

// address is a filter by account address, it could be principal or any affected address.
bytes address = 2;

@abergasov abergasov force-pushed the 3315-genesis-address-format_api_signature_change branch from cd8a4ad to f896c52 Compare July 15, 2022 18:30
@lrettig
Copy link
Copy Markdown
Collaborator

lrettig commented Jul 15, 2022

These are also addresses:

bytes principal = 2;
bytes template = 3;

Please check for others

@lrettig lrettig changed the title 3315 Genesis address format - change address type from byte slice to string Genesis address format - change address type from byte slice to string Jul 15, 2022
bytes principal = 2;
bytes template = 3;
AccountId principal = 2;
AccountId template = 3;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i don't know if template should have an hrp. this seems a bit weird. @lrettig what do you think?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It should. It's a domain-specific thing -- the same address (i.e., bytestr) can exist on many networks, but a template may only be deployed on one, and the HRP specifies the domain.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think we can make it optional for both principal and template since HRP is already implied by GENESISID, which the tx is bound to (spacemeshos/pm#135)

Comment thread proto/spacemesh/v1/tx_types.proto Outdated
Comment thread proto/spacemesh/v1/tx_types.proto Outdated
@abergasov abergasov merged commit 57864fb into master Aug 8, 2022
@abergasov abergasov deleted the 3315-genesis-address-format_api_signature_change branch August 8, 2022 08:19
bors Bot pushed a commit to spacemeshos/go-spacemesh that referenced this pull request Aug 16, 2022
## Motivation
<!-- Please mention the issue fixed by this PR or detailed motivation -->
Closes #3315
<!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged -->

## Changes
* address moved to separate pkg. probably in future will be as independent library
* address still array of bytes ([24]byte), but implement `bech32` algorithm. Additional first byte reserved for contain HumanReadablePart of address (now contain info about networkID - test|main net), so result is [25]byte
* all api endpoints will be changed to use string instead []byte (related pr spacemeshos/api#173)

## Test Plan
unit tests

## TODO
<!-- This section should be removed when all items are complete -->
- [ ] Explain motivation or link existing issue(s)
- [ ] Test changes and document test plan
- [ ] Update documentation as needed

## DevOps Notes
<!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases -->
- [ ] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources)
- [ ] This PR does not affect public APIs
- [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.)
- [x] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
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.

Genesis address format

3 participants