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

[BCI-1477] Implement cosmos specific txn type for generic txm #360

Draft
wants to merge 7 commits into
base: cosmos-generic-txm-staging
Choose a base branch
from

Conversation

yongkangc
Copy link
Contributor

@yongkangc yongkangc commented Aug 24, 2023

This PR implements implements chain specific transaction type based on the generic Transaction manager transaction interface.

Context:

Changes:

Note:

References:

@yongkangc yongkangc changed the title [1477] Implement cosmos specific transaction type for generic txm [1477] Implement cosmos specific txn type for generic txm Aug 24, 2023
@yongkangc yongkangc force-pushed the BCI-1477-Cosmos-Txn-Type branch from 82eb5e8 to 6ea8e59 Compare August 24, 2023 03:48
@yongkangc yongkangc force-pushed the BCI-1477-Cosmos-Txn-Type branch from 6ea8e59 to a1e7d63 Compare August 24, 2023 03:51
@yongkangc yongkangc changed the title [1477] Implement cosmos specific txn type for generic txm [BCI-1477] Implement cosmos specific txn type for generic txm Aug 24, 2023
return nil, errors.Errorf("unrecognized message type: %s", msgType)
}

// A wrapper for sdk.Address. Uses fixed max address size of 32-byte size to satisfy Hashable interface.

Choose a reason for hiding this comment

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

Just curious, why do we use 32-bytes, and not just []bytes?
As in this comment, I don't think the Hashable interface enforces us to have [32]byte.

Using just []byte would allow us to use sdk.Address directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would need to use 32-bytes because it needs to conform to the comparable constraint implemented by the hashable interface. Would update the comment to make this clearer


// Convert a generic txmgr Address to a Cosmos sdk.AccAddress
func ToCosmosAddress(addr Address) sdk.AccAddress {
return sdk.AccAddress(bytes.TrimRight(addr[:], "\x00"))

Choose a reason for hiding this comment

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

If I understand this correctly, does this remove all trailing 0s from the address?

What if the actual cosmos address had 0s in the address?
Or, I assume cosmos address wouldn't have 0s(unicode 0s, not string of "0") in the address, so its safe to do this?

Copy link
Contributor Author

@yongkangc yongkangc Aug 25, 2023

Choose a reason for hiding this comment

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

If I understand this correctly, does this remove all trailing 0s from the address?

yup, this will remove trailing zeros from the addresses.

What if the actual cosmos address had 0s in the address?

The cosmos addresses can have 0s in the address, but it would not have unicode 0s

Cosmos addresses are derived from public keys using the Bech32 encoding scheme. Because Bech32 uses a specific set of 32 characters, the Unicode null character (U+0000) would not be possible.

Reference:
Bech32 is an encoding scheme used to encode SegWit addresses and Lightning invoices. The Bech32 alphabet contains 32 characters, including lowercase letters a-z and the numbers 0-9, excluding the number 1 and the letters ‘b’, ‘i’, ‘o’ to avoid reader confusion. Bech32 also includes an error detection mechanism.

@yongkangc yongkangc changed the base branch from develop to cosmos-generic-txm-staging August 25, 2023 05:28
@yongkangc
Copy link
Contributor Author

The Staging branch has been changed to cosmos-generic-txm-staging

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.

2 participants