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

Add data commitment logic #222

Merged

Conversation

rach-id
Copy link
Member

@rach-id rach-id commented Feb 22, 2022

Adds data commitment logic.
Closes: #199

@rach-id rach-id marked this pull request as draft February 22, 2022 11:54
@rach-id
Copy link
Member Author

rach-id commented Feb 22, 2022

@evan-forbes Please take a look if this is in the right direction. And also couple questions when you have time:

@evan-forbes
Copy link
Member

evan-forbes commented Feb 22, 2022

What do you mean by: that the provided validator's bridge address is the one associated with that validator in this issue #199 ?

This just means that we have to effectively check that the signature provided is valid and is actually from the relevant eth address for that validator. This involves checking the current state to see what address is the one that is currently associated with that validator, and then verifying the evm friendly signature.

This is typically done in the gravity module via this func

https://github.com/Gravity-Bridge/Gravity-Bridge/blob/7a786d2ba999b45292c976282cf2aa88ba4cbba9/module/x/gravity/keeper/msg_server.go#L313-L353

@rach-id
Copy link
Member Author

rach-id commented Feb 22, 2022

@evan-forbes This means we need to add an extra field to DataCommitmentConfirm containing the eth_address ?
Or, we need to have a KV store for these, no ?

@evan-forbes
Copy link
Member

evan-forbes commented Feb 22, 2022

That's a good question, while I think we could do either, I think staying consistent with the gravity bridge and ValSetConfirm is worthwhile imho, so I'm in favor of adding the eth address field. One less time we have to access state seems reasonable and simple at first glance. Could be wrong tho, so will definitely leave it up to you.

@rach-id
Copy link
Member Author

rach-id commented Feb 22, 2022

Cool. Will try the former, and if it doesn't work, I will switch. Thanks

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

nice!

x/qgb/keeper/msg_server.go Outdated Show resolved Hide resolved
x/qgb/keeper/msg_server.go Outdated Show resolved Hide resolved
Copy link
Member Author

@rach-id rach-id left a comment

Choose a reason for hiding this comment

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

@evan-forbes Few questions when you have time please :D

app/app.go Show resolved Hide resolved
x/qgb/config.go Outdated Show resolved Hide resolved
x/qgb/config.go Outdated
config := sdk.GetConfig()
config.SetBech32PrefixForAccount(bech32PrefixAccAddr, bech32PrefixAccPub)
config.SetBech32PrefixForValidator(bech32PrefixValAddr, bech32PrefixValPub)
config.SetBech32PrefixForConsensusNode(bech32PrefixConsAddr, bech32PrefixConsPub)
Copy link
Member Author

Choose a reason for hiding this comment

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

Will we need also the consensus bech32 prefixes ?

Copy link
Member

Choose a reason for hiding this comment

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

while it might not be done using this exact mechanism, this is already done using celes, so we should avoid doing this again using qgb. Surprised this doesn't break anything lol

Copy link
Member Author

Choose a reason for hiding this comment

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

This is defined inside the module... maybe it is picked up per module?

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense for them to have it in their module, as they want to have grav addresses, but not ours as we will have celes. sdk.GetConfig is global (hence the "sealing"), and having different address types per module is not an sdk feature I'm aware of. The only addresses that we should have to manage are done so via the orchestrator and eth addresses, which we are keeping in the state.

Do things break when we remove them?

Copy link
Member Author

Choose a reason for hiding this comment

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

not at all. Ill remove them then.

}

prefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.DataCommitmentConfirmKey))
start, end := prefixRange([]byte(types.DataCommitmentConfirmKey)) // FIXME can we make this faster ?
Copy link
Member Author

Choose a reason for hiding this comment

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

This is supposed to be iterating over the whole data commitments store, right ?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, but am still ivestigating/thinking about this

@rach-id rach-id marked this pull request as ready for review February 24, 2022 17:01
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

good stuff 👍

x/qgb/config.go Outdated
config := sdk.GetConfig()
config.SetBech32PrefixForAccount(bech32PrefixAccAddr, bech32PrefixAccPub)
config.SetBech32PrefixForValidator(bech32PrefixValAddr, bech32PrefixValPub)
config.SetBech32PrefixForConsensusNode(bech32PrefixConsAddr, bech32PrefixConsPub)
Copy link
Member

Choose a reason for hiding this comment

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

while it might not be done using this exact mechanism, this is already done using celes, so we should avoid doing this again using qgb. Surprised this doesn't break anything lol

x/qgb/keeper/keeper.go Outdated Show resolved Hide resolved
x/qgb/keeper/msg_server.go Outdated Show resolved Hide resolved
x/qgb/keeper/query_data_commitment.go Outdated Show resolved Hide resolved
}

prefixStore := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.DataCommitmentConfirmKey))
start, end := prefixRange([]byte(types.DataCommitmentConfirmKey)) // FIXME can we make this faster ?
Copy link
Member

Choose a reason for hiding this comment

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

I think so, but am still ivestigating/thinking about this

x/qgb/keeper/query_data_commitment.go Outdated Show resolved Hide resolved
Comment on lines +74 to +80
for ; iterator.Valid(); iterator.Next() {
confirm := types.MsgDataCommitmentConfirm{}
k.cdc.MustUnmarshal(iterator.Value(), &confirm)
if confirm.BeginBlock <= beginBlock && confirm.EndBlock >= endBlock {
confirms = append(confirms, confirm)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

note to self: dig into this more

@rach-id
Copy link
Member Author

rach-id commented Feb 24, 2022

@evan-forbes If this PR is good. We can merge :D

Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

👍

@evan-forbes evan-forbes merged commit 1dc066b into celestiaorg:qgb-integration Feb 24, 2022
@rach-id rach-id deleted the add_data_commitment_logic branch February 24, 2022 23:19
rach-id added a commit to rach-id/celestia-app that referenced this pull request May 9, 2022
* adds msgs + query proto

* adds data commitment basic functions

* adds data commitment query skeleton + server emitting events

* linter changes

* adds data commitment keeper + basic queries

* persisting data commitment confirm on submission

* adds data commitment confirms by validator query

* adds verification for validator and ethereum address when handling data commitments confirm messages

* adds data commitment by range keeper + query

* adds comments

* adds staking keeper to qgb and GetOrchestratorValidator implementation

* adds delegate key query

* adds qgb module address config

* cosmetics

* removes unnecessary comment

* uses restricted version of staking keeper for QGB

* cosmetics

* removes config file
@rach-id rach-id self-assigned this May 9, 2022
@rach-id rach-id added the C: QGB label May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants