create TypeID for Actions/Auths + delete reflection in Registry #297

najeal · 2023-07-26T20:30:31Z

Closes #228

This PR adds GetTypeID method to the Action & Auth interfaces.

Now we register directly an ID instead of a Type in Registry.
Using a TypeID tied to the Action/Auth struct makes the reflection mechanism "legacy". Therefore it has been cleaned.

@patrick-ogrady is it what you expected?

…abs#228

patrick-ogrady · 2023-07-26T20:38:20Z

examples/tokenvm/actions/common.go

+package actions
+
+const (
+	burnAssetID uint8 = iota


Please list these out explicitly. iota will do the job but find it is much more delicate.

If there is duplicated id created manually, the registry will return the error 👍

May be worth adding that as a comment just for other contributors to keep in mind.

patrick-ogrady · 2023-07-26T20:39:43Z

@patrick-ogrady is it what you expected?

Reviewing now...this is exactly what I had in mind ❤️ . I didn't realize the function signatures would be so much cleaner, which is awesome.

patrick-ogrady · 2023-07-26T20:41:15Z

chain/transaction.go

@@ -334,22 +329,14 @@ func (t *Transaction) Payer() string {

 func (t *Transaction) Marshal(


nit: make this 1 line

patrick-ogrady · 2023-07-26T20:41:25Z

chain/transaction.go

 	t.Action.Marshal(p)
-	p.PackByte(authByte)
+	p.PackByte(authID)
 	t.Auth.Marshal(p)
 	return p.Err()
 }

 func MarshalTxs(


nit: make this 1 line

patrick-ogrady · 2023-07-26T21:11:16Z

Ran the CI to give you a read on what's remaining to change 👍

patrick-ogrady · 2023-07-26T22:12:15Z

codec/type_parser.go

-// LookupType returns the index, decoder function and the success of lookup of
-// type [o] from Typeparser [p].
-func (p *TypeParser[T, X, Y]) LookupType(o T) (uint8, func(*Packer, X) (T, error), Y, bool) {
-	index, ok := p.typeToIndex[fmt.Sprintf("%T", o)]


codec/type_parser.go

patrick-ogrady · 2023-07-26T22:19:32Z

examples/morpheusvm/actions/common.go

+package actions
+
+const (
+	// IDs explicitly listed. Registry will avoid usage of duplicated IDs by returning an error when occurs


nit (for all):

// Note: Registry will error during initialization if a duplicate ID is assigned. We explicitly assign IDs to avoid accidental remapping.

You can also put this just above const

patrick-ogrady · 2023-07-26T22:20:16Z

@najeal VERY close! just 1-2 small nits. Kicking off full CI rn.

patrick-ogrady · 2023-07-26T22:24:30Z

Would be good future work to convert this to use TypeID:

hypersdk/examples/tokenvm/controller/controller.go

Lines 177 to 211 in 51cee4e

    
           switch action := tx.Action.(type) { 
        
           case *actions.CreateAsset: 
        
           	c.metrics.createAsset.Inc() 
        
           case *actions.MintAsset: 
        
           	c.metrics.mintAsset.Inc() 
        
           case *actions.BurnAsset: 
        
           	c.metrics.burnAsset.Inc() 
        
           case *actions.ModifyAsset: 
        
           	c.metrics.modifyAsset.Inc() 
        
           case *actions.Transfer: 
        
           	c.metrics.transfer.Inc() 
        
           case *actions.CreateOrder: 
        
           	c.metrics.createOrder.Inc() 
        
           	actor := auth.GetActor(tx.Auth) 
        
           	c.orderBook.Add(tx.ID(), actor, action) 
        
           case *actions.FillOrder: 
        
           	c.metrics.fillOrder.Inc() 
        
           	orderResult, err := actions.UnmarshalOrderResult(result.Output) 
        
           	if err != nil { 
        
           		// This should never happen 
        
           		return err 
        
           	} 
        
           	if orderResult.Remaining == 0 { 
        
           		c.orderBook.Remove(action.Order) 
        
           		continue 
        
           	} 
        
           	c.orderBook.UpdateRemaining(action.Order, orderResult.Remaining) 
        
           case *actions.CloseOrder: 
        
           	c.metrics.closeOrder.Inc() 
        
           	c.orderBook.Remove(action.Order) 
        
           case *actions.ImportAsset: 
        
           	c.metrics.importAsset.Inc() 
        
           case *actions.ExportAsset: 
        
           	c.metrics.exportAsset.Inc() 
        
           }

patrick-ogrady · 2023-07-26T22:41:32Z

Looks like full CI passed, as soon as nits are pushed should be good to merge.

patrick-ogrady · 2023-07-26T23:02:08Z

(no need to update branch to main...will merge as soon as the sync tests pass)

najeal · 2023-07-26T23:05:57Z

Would be good future work to convert this to use TypeID:

hypersdk/examples/tokenvm/controller/controller.go

Lines 177 to 211 in 51cee4e

switch action := tx.Action.(type) {

case *actions.CreateAsset:

c.metrics.createAsset.Inc()

case *actions.MintAsset:

c.metrics.mintAsset.Inc()

case *actions.BurnAsset:

c.metrics.burnAsset.Inc()

case *actions.ModifyAsset:

c.metrics.modifyAsset.Inc()

case *actions.Transfer:

c.metrics.transfer.Inc()

case *actions.CreateOrder:

c.metrics.createOrder.Inc()

actor := auth.GetActor(tx.Auth)

c.orderBook.Add(tx.ID(), actor, action)

case *actions.FillOrder:

c.metrics.fillOrder.Inc()

orderResult, err := actions.UnmarshalOrderResult(result.Output)

if err != nil {

// This should never happen

return err

}

if orderResult.Remaining == 0 {

c.orderBook.Remove(action.Order)

continue

}

c.orderBook.UpdateRemaining(action.Order, orderResult.Remaining)

case *actions.CloseOrder:

c.metrics.closeOrder.Inc()

c.orderBook.Remove(action.Order)

case *actions.ImportAsset:

c.metrics.importAsset.Inc()

case *actions.ExportAsset:

c.metrics.exportAsset.Inc()

}

You want the same switch but on the TypeID instead?

patrick-ogrady · 2023-07-26T23:15:47Z

Would be good future work to convert this to use TypeID:

hypersdk/examples/tokenvm/controller/controller.go

Lines 177 to 211 in 51cee4e

switch action := tx.Action.(type) {

case *actions.CreateAsset:

c.metrics.createAsset.Inc()

case *actions.MintAsset:

c.metrics.mintAsset.Inc()

case *actions.BurnAsset:

c.metrics.burnAsset.Inc()

case *actions.ModifyAsset:

c.metrics.modifyAsset.Inc()

case *actions.Transfer:

c.metrics.transfer.Inc()

case *actions.CreateOrder:

c.metrics.createOrder.Inc()

actor := auth.GetActor(tx.Auth)

c.orderBook.Add(tx.ID(), actor, action)

case *actions.FillOrder:

c.metrics.fillOrder.Inc()

orderResult, err := actions.UnmarshalOrderResult(result.Output)

if err != nil {

// This should never happen

return err

}

if orderResult.Remaining == 0 {

c.orderBook.Remove(action.Order)

continue

}

c.orderBook.UpdateRemaining(action.Order, orderResult.Remaining)

case *actions.CloseOrder:

c.metrics.closeOrder.Inc()

c.orderBook.Remove(action.Order)

case *actions.ImportAsset:

c.metrics.importAsset.Inc()

case *actions.ExportAsset:

c.metrics.exportAsset.Inc()

}

You want the same switch but on the TypeID instead?

Yeah, the idea is to remove all use of reflect, especially in the "hot path". Do you want to do that here or in a separate PR?

najeal · 2023-07-26T23:18:09Z

@patrick-ogrady I will do it in another PR. I will also pick another issue in the list.

create TypeID for Actions/Auths + delete reflection in Registry ava-l…

20fd597

…abs#228

najeal requested a review from patrick-ogrady as a code owner July 26, 2023 20:30

patrick-ogrady reviewed Jul 26, 2023

View reviewed changes

patrick-ogrady changed the title ~~create TypeID for Actions/Auths + delete reflection in Registry #228~~ create TypeID for Actions/Auths + delete reflection in Registry Jul 26, 2023

najeal added 2 commits July 26, 2023 22:56

actions & auth: put ID values explicitly instead of using iota

c4543e7

function signature delete useless line

5af5a7e

resolve last lint issues

d6305e7

najeal requested a review from patrick-ogrady July 26, 2023 21:57

Merge branch 'main' into action-auth-id

dc0f379

patrick-ogrady reviewed Jul 26, 2023

View reviewed changes

codec/type_parser.go Outdated Show resolved Hide resolved

patrick-ogrady reviewed Jul 26, 2023

View reviewed changes

codec/type_parser.go Show resolved Hide resolved

patrick-ogrady reviewed Jul 26, 2023

View reviewed changes

patrick-ogrady added run load labels Jul 26, 2023

najeal added 2 commits July 27, 2023 00:52

reword Note

cd2d5e7

change register items limit

9d9cd40

patrick-ogrady approved these changes Jul 26, 2023

View reviewed changes

patrick-ogrady merged commit fe99c39 into ava-labs:main Jul 26, 2023
18 checks passed

najeal deleted the action-auth-id branch July 26, 2023 23:25

najeal mentioned this pull request Jul 27, 2023

avoid type assertion on Action/Auth in switchs #311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create TypeID for Actions/Auths + delete reflection in Registry #297

create TypeID for Actions/Auths + delete reflection in Registry #297

najeal commented Jul 26, 2023

patrick-ogrady Jul 26, 2023

najeal Jul 26, 2023

patrick-ogrady Jul 26, 2023

patrick-ogrady commented Jul 26, 2023 •

edited

Loading

patrick-ogrady Jul 26, 2023

patrick-ogrady Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

patrick-ogrady Jul 26, 2023

patrick-ogrady Jul 26, 2023

patrick-ogrady Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023 •

edited

Loading

najeal commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

najeal commented Jul 26, 2023

		@@ -334,22 +329,14 @@ func (t *Transaction) Payer() string {

		func (t *Transaction) Marshal(

create TypeID for Actions/Auths + delete reflection in Registry #297

create TypeID for Actions/Auths + delete reflection in Registry #297

Conversation

najeal commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-ogrady commented Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-ogrady commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick-ogrady commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023 • edited Loading

najeal commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023

najeal commented Jul 26, 2023

patrick-ogrady commented Jul 26, 2023 •

edited

Loading

patrick-ogrady commented Jul 26, 2023 •

edited

Loading