cerc-io/laconic-sdk
19
0
Fork 1
Code Issues 3 Pull Requests Actions Packages Projects Releases 17 Wiki Activity

fix: Test fail after PR#40 #15

Merged
0xmuralik merged 7 commits from murali/fix-tests into main 2023-01-09 06:47:09 +00:00
Conversation 13 Commits 7 Files Changed 39 +3144 -1808
0xmuralik commented 2022-12-19 06:15:50 +00:00 (Migrated from github.com)
Copy Link

closes: #8

closes: #8
nikugogoi (Migrated from github.com) reviewed 2022-12-19 06:15:50 +00:00
i-norden reviewed 2022-12-19 06:15:50 +00:00
0xmuralik commented 2022-12-27 09:17:20 +00:00 (Migrated from github.com)
Copy Link

@nikugogoi @roysc I made a few changes on laconic-sdk and laconicd (https://github.com/cerc-io/laconicd/pull/70)
Now I'm stuck with a different error
signature verification failed; please verify account number (0) and chain-id (ethermint_9000-1): feePayer pubkey EthPubKeySecp256k1{02E848C9FABEED18CF49F8315ED6B4D162487B2DAEA701414EBF8BD33CF943A531} is different from transaction pubkey EthPubKeySecp256k1{0358D2444AF562B806BDF14586DF061DD969E043C93B0A780B4E4834FF313A2E86}: invalid pubkey [cerc-io/laconicd/app/ante/eip712.go:251]: unauthorized

To recreate: checkout to murali/fix-tests on laconic-sdk and murali/record-attributes on laconicd

@nikugogoi @roysc I made a few changes on laconic-sdk and laconicd (https://github.com/cerc-io/laconicd/pull/70) Now I'm stuck with a different error `signature verification failed; please verify account number (0) and chain-id (ethermint_9000-1): feePayer pubkey EthPubKeySecp256k1{02E848C9FABEED18CF49F8315ED6B4D162487B2DAEA701414EBF8BD33CF943A531} is different from transaction pubkey EthPubKeySecp256k1{0358D2444AF562B806BDF14586DF061DD969E043C93B0A780B4E4834FF313A2E86}: invalid pubkey [cerc-io/laconicd/app/ante/eip712.go:251]: unauthorized` To recreate: checkout to murali/fix-tests on laconic-sdk and murali/record-attributes on laconicd
dboreham commented 2022-12-27 17:31:52 +00:00
Owner
Copy Link

@nikugogoi @roysc I made a few changes on laconic-sdk and laconicd (cerc-io/laconicd#70) Now I'm stuck with a different error signature verification failed; please verify account number (0) and chain-id (ethermint_9000-1): feePayer pubkey EthPubKeySecp256k1{02E848C9FABEED18CF49F8315ED6B4D162487B2DAEA701414EBF8BD33CF943A531} is different from transaction pubkey EthPubKeySecp256k1{0358D2444AF562B806BDF14586DF061DD969E043C93B0A780B4E4834FF313A2E86}: invalid pubkey [cerc-io/laconicd/app/ante/eip712.go:251]: unauthorized

To recreate: checkout to murali/fix-tests on laconic-sdk and murali/record-attributes on laconicd

Hi, just noticed the message above references chain id ethermint_9000-1 -- in my scripts I changed it to laconic_9000-1.

> @nikugogoi @roysc I made a few changes on laconic-sdk and laconicd ([cerc-io/laconicd#70](https://github.com/cerc-io/laconicd/pull/70)) Now I'm stuck with a different error `signature verification failed; please verify account number (0) and chain-id (ethermint_9000-1): feePayer pubkey EthPubKeySecp256k1{02E848C9FABEED18CF49F8315ED6B4D162487B2DAEA701414EBF8BD33CF943A531} is different from transaction pubkey EthPubKeySecp256k1{0358D2444AF562B806BDF14586DF061DD969E043C93B0A780B4E4834FF313A2E86}: invalid pubkey [cerc-io/laconicd/app/ante/eip712.go:251]: unauthorized` > > To recreate: checkout to murali/fix-tests on laconic-sdk and murali/record-attributes on laconicd Hi, just noticed the message above references chain id ethermint_9000-1 -- in my scripts I changed it to laconic_9000-1.
0xmuralik commented 2022-12-28 04:38:35 +00:00 (Migrated from github.com)
Copy Link

Hi, just noticed the message above references chain id ethermint_9000-1 -- in my scripts I changed it to laconic_9000-1.

As long as the chai-ID is same in both the repos, it works. Both laconicd and laconic-sdk in my PRs use ethermint chainID.
Now, only SetRecord Messages return this error. All other messages work as expected.

> Hi, just noticed the message above references chain id ethermint_9000-1 -- in my scripts I changed it to laconic_9000-1. As long as the chai-ID is same in both the repos, it works. Both laconicd and laconic-sdk in my PRs use ethermint chainID. Now, only SetRecord Messages return this error. All other messages work as expected.
AFDudley commented 2022-12-28 04:45:08 +00:00
Owner
Copy Link

Can we please set the chain ID to laconic instead of ethermint? Thanks!

Can we please set the chain ID to laconic instead of ethermint? Thanks!
dboreham commented 2022-12-28 04:54:22 +00:00
Owner
Copy Link

Hi, just noticed the message above references chain id ethermint_9000-1 -- in my scripts I changed it to laconic_9000-1.

As long as the chai-ID is same in both the repos, it works. Both laconicd and laconic-sdk in my PRs use ethermint chainID. Now, only SetRecord Messages return this error. All other messages work as expected.

That's the wrong chain id. It should be changed in laconicd, and left as-is in laconic-sdk. See : https://github.com/cerc-io/laconicd/pull/71/files#r1058056971

> > Hi, just noticed the message above references chain id ethermint_9000-1 -- in my scripts I changed it to laconic_9000-1. > > As long as the chai-ID is same in both the repos, it works. Both laconicd and laconic-sdk in my PRs use ethermint chainID. Now, only SetRecord Messages return this error. All other messages work as expected. That's the wrong chain id. It should be changed in laconicd, and left as-is in laconic-sdk. See : https://github.com/cerc-io/laconicd/pull/71/files#r1058056971
dboreham commented 2022-12-28 04:58:55 +00:00
Owner
Copy Link

This seems like quite a large number of files changed in order to resolve a test regression. Could you give a bit of background on what's going on in this PR? (or does this PR incorporate some other body of work not directly related to the test fix perhaps)?

This seems like quite a large number of files changed in order to resolve a test regression. Could you give a bit of background on what's going on in this PR? (or does this PR incorporate some other body of work not directly related to the test fix perhaps)?
0xmuralik commented 2022-12-28 05:10:29 +00:00 (Migrated from github.com)
Copy Link

Can we please set the chain ID to laconic instead of ethermint? Thanks!

Fixed that in a different open PR https://github.com/cerc-io/laconicd/pull/69

> Can we please set the chain ID to laconic instead of ethermint? Thanks! Fixed that in a different open PR https://github.com/cerc-io/laconicd/pull/69
AFDudley commented 2022-12-28 05:11:06 +00:00
Owner
Copy Link

Thank you.

Thank you.
0xmuralik commented 2022-12-28 05:19:28 +00:00 (Migrated from github.com)
Copy Link

This seems like quite a large number of files changed in order to resolve a test regression. Could you give a bit of background on what's going on in this PR? (or does this PR incorporate some other body of work not directly related to the test fix perhaps)?

Most of the changed files are due to renaming of nameservice module to "registry". You can go through the proto/vulcanize which are same as in laconicd and ignore all the src/proto files which are proto generated.
Now the main changes are refferred to the change of attributes type from string to Any. e295b8de36/proto/vulcanize/registry/v1beta1/registry.proto (L67)

See:
src/account.ts
src/messages/registry.ts
src/types.ts

> This seems like quite a large number of files changed in order to resolve a test regression. Could you give a bit of background on what's going on in this PR? (or does this PR incorporate some other body of work not directly related to the test fix perhaps)? Most of the changed files are due to renaming of nameservice module to "registry". You can go through the proto/vulcanize which are same as in laconicd and ignore all the src/proto files which are proto generated. Now the main changes are refferred to the change of attributes type from string to Any. https://github.com/cerc-io/laconic-sdk/blob/e295b8de3682da9d21b2b8d6c88cb8ca24078d21/proto/vulcanize/registry/v1beta1/registry.proto#L67 See: src/account.ts src/messages/registry.ts src/types.ts
0xmuralik commented 2022-12-28 11:02:33 +00:00 (Migrated from github.com)
Copy Link

Update:
Problem 1:
After hardcodeing record attributes in https://github.com/cerc-io/laconicd/pull/70 I found out that the issue was with EIP712 signature verification.
While unpacking the messages to generate typed data, the attributes field in record, which is of type Any, is also being unpacked to the typ of attributes Ex: Website Registration Record Attributes
But the on the client side the message with attributes as type Any is signed. And thus the difference in signBytes on laconicd and laconic-sdk.

Problem 2:
The tests on laconic-sdk rely on versioning of records but the attribute types defined on laconicd do not have a version field. So when the record is signed on laconic-sdk and the signature is added to the payload, the signedbytes are of the record containing a version field. When we try to verify the owner of the record from signature inside the keeper, the signBytes generated there are of the record that do not have a version field and the verification of record owner fails.

Update: Problem 1: After hardcodeing record attributes in https://github.com/cerc-io/laconicd/pull/70 I found out that the issue was with EIP712 signature verification. While unpacking the messages to generate typed data, the attributes field in record, which is of type Any, is also being unpacked to the typ of attributes Ex: Website Registration Record Attributes But the on the client side the message with attributes as type Any is signed. And thus the difference in signBytes on laconicd and laconic-sdk. Problem 2: The tests on laconic-sdk rely on versioning of records but the attribute types defined on laconicd do not have a version field. So when the record is signed on laconic-sdk and the signature is added to the payload, the signedbytes are of the record containing a version field. When we try to verify the owner of the record from signature inside the keeper, the signBytes generated there are of the record that do not have a version field and the verification of record owner fails.
👍 1
0xmuralik commented 2023-01-02 11:05:42 +00:00 (Migrated from github.com)
Copy Link

I fixed the above problems and 35/40 tests are passing. Working on the failing tests now.

I fixed the above problems and 35/40 tests are passing. Working on the failing tests now.
0xmuralik commented 2023-01-03 09:38:22 +00:00 (Migrated from github.com)
Copy Link

checkout to murali/record-attributes on laconicd before running tests
except for one util test which was failing even before PR #40 everything else passes

checkout to murali/record-attributes on laconicd before running tests except for one util test which was failing even before PR #40 everything else passes
0xmuralik commented 2023-01-09 06:46:57 +00:00 (Migrated from github.com)
Copy Link

All tests pass successfully and merging this now.

All tests pass successfully and merging this now.
Sign in to join this conversation.
Reviewers
No reviewers
i-norden
nikugogoi
Labels
Clear labels   
bug

Something isn't working

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
in progress

   
invalid

This doesn't seem right

  
question

Further information is requested

  
wontfix

This will not be worked on

  
Copied from Github

An issue or PR manually copied from GitHub.

  
Kind/Breaking

Breaking change that won't be backward compatible

  
Kind/Bug

Something is not working

  
Kind/Documentation

Documentation changes

  
Kind/Enhancement

Improve existing functionality

  
Kind/Feature

New functionality

  
Kind/Security

This is security issue

  
Kind/Testing

Issue or pull request related to testing

  
Priority
Critical
The priority is critical

  
Priority
High
The priority is high

  
Priority
Low
The priority is low

  
Priority
Medium
The priority is medium

  
Reviewed
Confirmed
Issue has been confirmed

  
Reviewed
Duplicate
This issue or pull request already exists

  
Reviewed
Invalid
Invalid issue

  
Reviewed
Won't Fix
This issue won't be fixed

  
Status
Abandoned
Somebody has started to work on this but abandoned work

  
Status
Blocked
Something is blocking this issue or pull request

  
Status
Need More Info
Feedback is required to reproduce issue or to continue work

No Label
bug
documentation
duplicate
enhancement
good first issue
help wanted
in progress
invalid
question
wontfix
Copied from Github
Kind/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/laconic-sdk#15
No description provided.
Delete Branch "murali/fix-tests"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?