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 key_version to sign call #4

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Conversation

bh2smith
Copy link
Contributor

@bh2smith bh2smith commented Apr 3, 2024

If approved, this PR closes #3.

The key_version field was made mandatory on the MPC-recovery contract (cf near/mpc#509). So now this contract needs to pass along key version.It was suggested to simply hard code the value in the call-site, but we also had to update function signatures.

Note that we also fixed some unrelated broken tests and the changes have been made of two commits (for easier reviewing).

I would also be willing to add a CI file to this project (in a follow up PR)

cc @encody

gas_station/src/lib.rs Outdated Show resolved Hide resolved
@firatNEAR firatNEAR requested a review from encody April 3, 2024 14:18
@encody
Copy link
Contributor

encody commented Apr 3, 2024

Thanks for this! My understanding is that, for now, there is only one key version. Since we are currently completely reworking the key management system, would it be workable to just... hardcode the key version in the MPC contract sign call?

@bh2smith
Copy link
Contributor Author

bh2smith commented Apr 3, 2024

hardcode the key version in the MPC contract sign call?

Yea that would make the changes here much less invasive.

Also what do you think about me splitting the PR into:

  1. Fix broken tests
  2. Hardcode key_version into sign call?

@bh2smith
Copy link
Contributor Author

bh2smith commented Apr 3, 2024

Ok, so I hard coded the key_version of 1. and made way fewer changes overall. Unfortunately one of these tests is now failing. I need to dig into it.

@bh2smith bh2smith marked this pull request as draft April 3, 2024 17:35
@bh2smith bh2smith force-pushed the 3/add-key-version branch from 06318c0 to 75414cd Compare April 3, 2024 17:43
@bh2smith bh2smith changed the base branch from master to feat/nft-key-integration April 3, 2024 17:43
@bh2smith bh2smith changed the base branch from feat/nft-key-integration to master April 3, 2024 17:43
@bh2smith bh2smith force-pushed the 3/add-key-version branch from 75414cd to 46bd0cf Compare April 3, 2024 17:47
@bh2smith
Copy link
Contributor Author

bh2smith commented Apr 3, 2024

I split the PR into two. The test fixing should be merged first so I can baser this on there and test the real changes.

#5

@bh2smith bh2smith marked this pull request as ready for review April 3, 2024 18:57
@bh2smith bh2smith force-pushed the 3/add-key-version branch from 3c72a0d to 76af6bb Compare April 3, 2024 18:58
@encody
Copy link
Contributor

encody commented Apr 4, 2024

Looks awesome, thanks!

@encody encody merged commit 52c6c10 into near:master Apr 4, 2024
@bh2smith bh2smith deleted the 3/add-key-version branch April 4, 2024 06:19
@bh2smith bh2smith mentioned this pull request Apr 4, 2024
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.

Missing Required Field key_version on MPC Contract
2 participants