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

Implement PrimaryKey for &Addr by removing unused parse_key #264

Closed

Conversation

webmaster128
Copy link
Member

The trait PrimaryKey puts a lot of requirements on the type implementing it. It requires zero-copy serialization and zero-copy deserialization.

However, deserializing the bytes into the key type is never used. If we remove it, we can easily implemement PrimaryKey for &Addr.


Do not merge as is. Target branch is a copy of the fork from #260.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

I like the simplicity of this. I think parse_key is not used because we're implementing parsing of keys when we need it using custom helper functions.

contracts/cw3-fixed-multisig/src/contract.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member

I think parse_key is not used because we're implementing parsing of keys when we need it using custom helper functions.

There is an open issue to make use of parse_key: #198 and thus avoid the helper functions.

It does simplify things to remove the call. Let's discuss whether we want to make #198 impossible first. If that is fine, let's do this approach. Otherwise, let's do the &K approach, which is quite nice

@maurolacy
Copy link
Contributor

maurolacy commented Apr 12, 2021

I think parse_key is not used because we're implementing parsing of keys when we need it using custom helper functions.

There is an open issue to make use of parse_key: #198 and thus avoid the helper functions.

My understanding is that #198 can be done independently of parse_key (at least, implicitly, i.e. automatically). Anyway, consolidating parse_key functionality wouldn't be bad.

It does simplify things to remove the call. Let's discuss whether we want to make #198 impossible first. If that is fine, let's do this approach. Otherwise, let's do the &K approach, which is quite nice

I agree #265 looks like the way to go. If we're OK with tolerating some syntax issues / constructs (mostly in test code), I would say, let's go for it.

@ethanfrey
Copy link
Member

I'm happy to merge this.

Let's add the parse_key when we want to take on #198 and we can do #265 at the same time, with a real benefit to such work

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Good solution.

@ethanfrey
Copy link
Member

Part was merged in #267

The rest cherry-picked into #260

All code used, just not this PR

@ethanfrey ethanfrey closed this Apr 12, 2021
@ethanfrey ethanfrey deleted the primary-key-addr branch August 26, 2021 09:27
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.

3 participants