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.

Sign up for GitHub

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

Jump to bottom

feat: add ApproveOracleRegistration tx #527

Merged
merged 24 commits into from
Dec 9, 2022
Merged

feat: add ApproveOracleRegistration tx #527

merged 24 commits into from
Dec 9, 2022

Conversation

audtlr24
Copy link
Contributor

@audtlr24 audtlr24 commented Dec 7, 2022

closes #510.

gyuguen and others added 14 commits December 2, 2022 10:22
@gyuguen
feat: define skeleton proto files
b390fc3
@gyuguen
fix
4a8176f
@gyuguen
fix
af16643
@gyuguen
fix
31f3b7d
@gyuguen
fix
d359d68
@gyuguen
fix
e189f52
@gyuguen
fix
8eccb41
@gyuguen
fix
eeee097
@audtlr24
feat: add skeleton code for oracle module
7c6b72e
@audtlr24
fix
1c32ec0
@audtlr24
feat: add ApproveOracleRegistration keeper function
a28fd6d
@audtlr24
Merge remote-tracking branch 'origin/main' into ft/510/approve_oracle…
6e16345 
…_registration_keeper

# Conflicts:
#	.github/workflows/ci.yml
#	proto/panacea/datadeal/v2/deal.proto
#	proto/panacea/datadeal/v2/genesis.proto
#	proto/panacea/datadeal/v2/query.proto
#	proto/panacea/datadeal/v2/tx.proto
#	proto/panacea/oracle/v2/oracle.proto
#	x/datadeal/types/deal.pb.go
#	x/datadeal/types/genesis.pb.go
#	x/datadeal/types/query.pb.go
#	x/datadeal/types/tx.pb.go
#	x/oracle/types/keys.go
#	x/oracle/types/oracle.pb.go
#	x/oracle/types/params.go
@audtlr24
feat: add test code
355133d
@audtlr24
feat: add tx cli & failed test code
29ee7ee
@audtlr24 audtlr24 self-assigned this Dec 7, 2022
@audtlr24 audtlr24 added this to the v2.1.0-beta milestone Dec 7, 2022
audtlr24
audtlr24 commented Dec 7, 2022
View reviewed changes
Copy link
Contributor Author

@audtlr24 audtlr24 left a comment

Choose a reason for hiding this comment

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

I use secp256k1 based signature verification. However, there Is another option that using btcec signing and verification. Please comment on which method would be better.

x/oracle/keeper/oracle.go Outdated Show resolved Hide resolved
x/oracle/keeper/oracle_test.go Outdated Show resolved Hide resolved
@gyuguen
Copy link
Contributor

gyuguen commented Dec 7, 2022 •
edited
Loading

I use secp256k1 based signature verification. However, there Is another option that using btcec signing and verification. Please comment on which method would be better.

If using the btcec signature, oracle and panacea-core must use the same btcec.
Or if you use secp256k1, oracle and panacea-core also use secp256k1.

using secp256k1(github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1)

oraclePrivKeySecp256k1 := secp256k1.GenPrivKey()
signatureBz, err := oraclePrivKeySecp256k1.Sign(unsignedCertBz)

using btcec

oraclePrivKeyBtcec := btcec.NewPrivateKey(btcec.S256())
signature, err := oraclePrivKeyBtcec..Sign(unsignedCertBz)
signatureBz := signature.Serialize()

The two signatures are different.

@gyuguen
Copy link
Contributor

gyuguen commented Dec 7, 2022

I use secp256k1 based signature verification. However, there Is another option that using btcec signing and verification. Please comment on which method would be better.

If using the btcec signature, oracle and panacea-core must use the same btcec. Or if you use secp256k1, oracle and panacea-core also use secp256k1.

using secp256k1(github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1)

oraclePrivKeySecp256k1 := secp256k1.GenPrivKey()
signatureBz, err := oraclePrivKeySecp256k1.Sign(unsignedCertBz)

using btcec

oraclePrivKeyBtcec := btcec.NewPrivateKey(btcec.S256())
signature, err := oraclePrivKeyBtcec..Sign(unsignedCertBz)
signatureBz := signature.Serialize()

The two signatures are different.

We discussed offline and decided to use btcec

youngjoon-lee
youngjoon-lee reviewed Dec 7, 2022
View reviewed changes
Copy link
Contributor

@youngjoon-lee youngjoon-lee left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems that a base branch of this PR should be set as PR #519. I can see many duplicated codes in the x/oracle/keeper/oracle.go which are already implemented by PR #519.

x/oracle/client/cli/txApproveOracleRegistration.go Outdated Show resolved Hide resolved
x/oracle/keeper/msg_server_oracle.go Outdated Show resolved Hide resolved
inchori
inchori reviewed Dec 7, 2022
View reviewed changes
Copy link

@inchori inchori left a comment

Choose a reason for hiding this comment

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

Thanks for your work. I left a minor comment.

x/oracle/client/cli/txApproveOracleRegistration.go Outdated Show resolved Hide resolved
@audtlr24 audtlr24 mentioned this pull request Dec 7, 2022
Merged
audtlr24 and others added 3 commits December 7, 2022 17:48
@audtlr24
Update x/oracle/keeper/msg_server_oracle.go
160a4d9 
Co-authored-by: Youngjoon Lee <yjlee@medibloc.org>
@audtlr24
feat: remove ApproveOracleRegistration CLI
51da4c2
@audtlr24
Merge remote-tracking branch 'origin/main' into ft/510/approve_oracle…
338344c 
…_registration_keeper

# Conflicts:
#	x/oracle/keeper/oracle.go
#	x/oracle/keeper/oracle_test.go
#	x/oracle/types/errors.go
#	x/oracle/types/events.go
#	x/oracle/types/message_oracle.go
#	x/oracle/types/oracle.go
gyuguen
gyuguen reviewed Dec 8, 2022
View reviewed changes
Copy link
Contributor

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

Thank you.
Check my review please.:)

x/oracle/keeper/oracle.go Outdated Show resolved Hide resolved
x/oracle/keeper/oracle.go Show resolved Hide resolved
x/oracle/types/events.go Outdated Show resolved Hide resolved
0xHansLee
0xHansLee approved these changes Dec 8, 2022
View reviewed changes
x/oracle/keeper/oracle.go Outdated Show resolved Hide resolved
x/oracle/keeper/oracle.go Show resolved Hide resolved
x/oracle/types/events.go Outdated Show resolved Hide resolved
inchori
inchori approved these changes Dec 9, 2022
View reviewed changes
Copy link

@inchori inchori left a comment

Choose a reason for hiding this comment

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

LGTM

gyuguen
gyuguen approved these changes Dec 9, 2022
View reviewed changes
Copy link
Contributor

@gyuguen gyuguen left a comment

Choose a reason for hiding this comment

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

LGTM

@audtlr24 audtlr24 merged commit eb408fc into main Dec 9, 2022
@audtlr24 audtlr24 deleted the ft/510/approve_oracle_registration_keeper branch December 9, 2022 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngjoon-lee youngjoon-lee youngjoon-lee left review comments

@gyuguen gyuguen gyuguen approved these changes

@0xHansLee 0xHansLee 0xHansLee approved these changes

@inchori inchori inchori approved these changes

@cl9200 cl9200 Awaiting requested review from cl9200 cl9200 is a code owner

Assignees

@audtlr24 audtlr24

Labels
None yet
Projects
None yet
Milestone
v2.1.0-alpha.3
Development

Successfully merging this pull request may close these issues.

Implement ApproveOracleRegistration tx
5 participants
@audtlr24 @gyuguen @youngjoon-lee @0xHansLee @inchori