WIP: Make notary support enabled by default in morph client #961

Closed
elebedeva wants to merge 2 commits from elebedeva/frostfs-node:feat/morph-default-notary-support into master
Member

Close #905

Notary support is enabled by default in morph client constructor. EnableNotarySupport() is marked as deprecated. Added DisableNotarySupport() method. Replaced panics with returned errors.

Tested with dev-env.

Signed-off-by: Ekaterina Lebedeva ekaterina.lebedeva@yadro.com

Close #905 Notary support is enabled by default in morph client constructor. `EnableNotarySupport()` is marked as `deprecated`. Added `DisableNotarySupport()` method. Replaced panics with returned errors. Tested with `dev-env`. Signed-off-by: Ekaterina Lebedeva <ekaterina.lebedeva@yadro.com>
elebedeva force-pushed feat/morph-default-notary-support from acd57e376a to 13645804b8 2024-02-06 15:11:45 +00:00 Compare
elebedeva force-pushed feat/morph-default-notary-support from 13645804b8 to a9b564a27e 2024-02-06 15:22:02 +00:00 Compare
elebedeva changed title from WIP: Make notary support enabled by default in morph client to Make notary support enabled by default in morph client 2024-02-06 15:35:01 +00:00
elebedeva requested review from storage-core-committers 2024-02-06 15:35:15 +00:00
elebedeva requested review from storage-core-developers 2024-02-06 15:35:16 +00:00
dstepanov-yadro reviewed 2024-02-06 15:41:33 +00:00
@ -72,15 +72,6 @@ func initMorphComponents(ctx context.Context, c *cfg) {
lookupScriptHashesInNNS(c) // smart contract auto negotiation
if c.cfgMorph.notaryEnabled {

c.cfgMorph.notaryEnabled is used after this? Can be deleted?

`c.cfgMorph.notaryEnabled` is used after this? Can be deleted?
Author
Member

replaced c.cfgMorph.notaryEnabled with c.cfgMorph.notaryDisabled, it is used to skip notary-related steps in non-notary environments.

replaced `c.cfgMorph.notaryEnabled` with `c.cfgMorph.notaryDisabled`, it is used to skip notary-related steps in non-notary environments.
dstepanov-yadro marked this conversation as resolved
fyrchik reviewed 2024-02-06 16:52:27 +00:00
@ -130,3 +122,4 @@
if !s.mainNotaryConfig.disabled {
// enable notary support in the main client
//lint:ignore SA1019 we need notary support enabled in main chain
Owner

What is the problem here?

What is the problem here?
Author
Member

I thought it was an appropriate case of using EnableNotarySupport() even if it's deprecated. Now it seems illogical (having in mind that if notary support is disabled - it must be on purpose) so i decided to return an error here instead.

I thought it was an appropriate case of using `EnableNotarySupport()` even if it's deprecated. Now it seems illogical (having in mind that if notary support is disabled - it must be on purpose) so i decided to return an `error` here instead.
fyrchik marked this conversation as resolved
@ -287,3 +338,3 @@
if c.notary == nil {
panic(notaryNotEnabledPanicMsg)
return errors.New(notaryNotEnabledErrMsg)
Owner

What about a global var ErrNotaryNotEnabled = errors.New? It could exist alongside the message constant, if it is used anywhere else.

What about a global `var ErrNotaryNotEnabled = errors.New`? It could exist alongside the message constant, if it is used anywhere else.
Author
Member

fixed.

fixed.
fyrchik marked this conversation as resolved
elebedeva force-pushed feat/morph-default-notary-support from a9b564a27e to e1301c90da 2024-02-20 08:57:38 +00:00 Compare
elebedeva force-pushed feat/morph-default-notary-support from e1301c90da to db61e5448a 2024-02-20 09:15:48 +00:00 Compare
acid-ant reviewed 2024-02-20 09:18:36 +00:00
@ -83,3 +75,2 @@
c.log.Info(logs.FrostFSNodeNotarySupport,
zap.Bool("sidechain_enabled", c.cfgMorph.notaryEnabled),
zap.Bool("sidechain_enabled", !c.cfgMorph.notaryDisabled),
Member

Why not to rename sidechain_enabled too?

Why not to rename `sidechain_enabled` too?
Author
Member

fixed.

fixed.
acid-ant marked this conversation as resolved
@ -78,6 +81,43 @@ func defaultNotaryConfig(c *Client) *notaryCfg {
// EnableNotarySupport creates notary structure in client that provides
// ability for client to get alphabet keys from committee or provided source
// and use proxy contract script hash to create tx for notary contract.
func (c *Client) enableNotarySupport(opts ...NotaryOption) error {
Member

What the difference with EnableNotarySupport?

What the difference with `EnableNotarySupport`?
Author
Member

It's the same EnableNotarySupport() but now it's called from Client constructor therefore there is no need to make it public.

It's the same `EnableNotarySupport()` but now it's called from `Client constructor` therefore there is no need to make it public.
Member

Why not to remove it or call private from public? Why we need to duplicate code?

Why not to remove it or call private from public? Why we need to duplicate code?
Author
Member

@aarifullin mentions it here:

To [enable notary support by default in morph client constructor], we need to invoke EnableNotarySupport in constructor, but at the same it would be better to make this method deprecated to notify client to stop using this invocation explictly.

I suggest to create enableNotarySupport, copy the definition to there

@aarifullin mentions it [here](https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/905): > To [enable notary support by default in morph client constructor], we need to invoke `EnableNotarySupport` in constructor, but at the same it would be better to make this method `deprecated` to notify client to stop using this invocation explictly. > I suggest to create `enableNotarySupport`, copy the definition to there
Member

Let's do like @aarifullin suggested:

func (c *Client) EnableNotarySupport(opts ...NotaryOption) error {
  return c.enableNotarySupport(opts...) // also use in the constructor
}
Let's do like @aarifullin suggested: ``` func (c *Client) EnableNotarySupport(opts ...NotaryOption) error { return c.enableNotarySupport(opts...) // also use in the constructor } ```
Author
Member

fixed.

fixed.
acid-ant marked this conversation as resolved
elebedeva force-pushed feat/morph-default-notary-support from db61e5448a to 519e988798 2024-02-20 12:42:53 +00:00 Compare
elebedeva force-pushed feat/morph-default-notary-support from 519e988798 to 62e2e8881e 2024-02-20 12:49:43 +00:00 Compare
dstepanov-yadro approved these changes 2024-02-20 13:06:31 +00:00
elebedeva force-pushed feat/morph-default-notary-support from 62e2e8881e to ac36ae3c23 2024-02-20 15:30:33 +00:00 Compare
elebedeva force-pushed feat/morph-default-notary-support from ac36ae3c23 to 318a23d820 2024-02-20 15:48:41 +00:00 Compare
elebedeva force-pushed feat/morph-default-notary-support from 318a23d820 to ff0f029788 2024-02-20 15:50:27 +00:00 Compare
acid-ant approved these changes 2024-02-20 16:12:17 +00:00
aarifullin approved these changes 2024-02-21 10:34:02 +00:00
fyrchik approved these changes 2024-02-21 11:36:07 +00:00
@ -137,3 +123,1 @@
if err != nil {
return fmt.Errorf("could not enable main chain notary support: %w", err)
}
return fmt.Errorf("main chain notary support is disabled")
Owner

Either the condition or the message is wrong, in this branch notary is enabled, but the message says it is disabled, why so?

Also, we could use errors.New here.

Either the condition or the message is wrong, in this branch notary is enabled, but the message says it is disabled, why so? Also, we could use `errors.New` here.
Owner

Was it tested on dev-env? (container creation is enough)

Was it tested on dev-env? (container creation is enough)
elebedeva changed title from Make notary support enabled by default in morph client to WIP: Make notary support enabled by default in morph client 2024-02-21 16:26:18 +00:00
Author
Member

While implementing this feature faced next problem in innerring:

  1. initialization of morph client now requires proxy contract

    • notary support is enabled in client's constructor
    • notary options must be passed to constructor
    • proxy contract is required notary option
  2. initialization of contracts requires morph client

Closing PR until solution is found.

While implementing this feature faced next problem in innerring: 1. initialization of morph client now requires proxy contract - notary support is enabled in client's constructor - notary options must be passed to constructor - proxy contract is required notary option 2. initialization of contracts requires morph client Closing PR until solution is found.
elebedeva closed this pull request 2024-03-26 08:11:49 +00:00
All checks were successful
DCO action / DCO (pull_request) Successful in 3m14s
Required
Details
Build / Build Components (1.21) (pull_request) Successful in 4m44s
Required
Details
Build / Build Components (1.20) (pull_request) Successful in 4m51s
Required
Details
Tests and linters / Lint (pull_request) Successful in 6m2s
Required
Details
Vulncheck / Vulncheck (pull_request) Successful in 10m18s
Required
Details
Tests and linters / Staticcheck (pull_request) Successful in 13m26s
Required
Details
Tests and linters / Tests (1.20) (pull_request) Successful in 13m47s
Required
Details
Tests and linters / Tests (1.21) (pull_request) Successful in 13m59s
Required
Details
Tests and linters / Tests with -race (pull_request) Successful in 3m35s
Required
Details

Pull request closed

Sign in to join this conversation.
No milestone
No project
No assignees
5 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: TrueCloudLab/frostfs-node#961
No description provided.