cburgdorf Github contribution chart
cburgdorf Github Stats
cburgdorf Most Used Languages

Activity

28 Sep 2022

Cburgdorf

Struct pattern

Add struct pattern

Add a struct pattern that can be used in match statements. e.g.,

struct MyS {
    pub x: i32
    pub b: bool

    fn eval(self) -> i32 {
        match self {
            MyS {x: x, b: true} => {
                return x
            }
            MyS {x: x, b: false} => {
                return -x
            }
        }
    }
} 

NOTE

  1. Currently, field labels are required even if a binding name is the same as the field name.
  2. To bind a field, the fields must be pub. Otherwise, the compiler complains about it.

FIx #794

enum Accumulator {
    Zero
    Int(u32)
    Clo(i32, i32)
}

struct MyStruct {
    pub acc: Accumulator
    pub y: i32
}

contract Bar {
} 

I noticed the above example still causes an ICE even in the current master. In #798, I overlooked that we should modify codegen/abi.rs so that non-emittable structs are excluded from abi events. I fixed the problem in this PR for testing a nested struct pattern.

  • [x] OPTIONAL: Update Spec if applicable

  • [x] Add entry to the release notes (may forgo for trivial changes)

  • [x] Clean up commit history

Merged On 28 Sep 2022 at 02:17:25

Cburgdorf

Great stuff!

Commented On 28 Sep 2022 at 02:17:25

Cburgdorf

Struct pattern

Add struct pattern

Add a struct pattern that can be used in match statements. e.g.,

struct MyS {
    pub x: i32
    pub b: bool

    fn eval(self) -> i32 {
        match self {
            MyS {x: x, b: true} => {
                return x
            }
            MyS {x: x, b: false} => {
                return -x
            }
        }
    }
} 

NOTE

  1. Currently, field labels are required even if a binding name is the same as the field name.
  2. To bind a field, the fields must be pub. Otherwise, the compiler complains about it.

FIx #794

enum Accumulator {
    Zero
    Int(u32)
    Clo(i32, i32)
}

struct MyStruct {
    pub acc: Accumulator
    pub y: i32
}

contract Bar {
} 

I noticed the above example still causes an ICE even in the current master. In #798, I overlooked that we should modify codegen/abi.rs so that non-emittable structs are excluded from abi events. I fixed the problem in this PR for testing a nested struct pattern.

  • [x] OPTIONAL: Update Spec if applicable

  • [x] Add entry to the release notes (may forgo for trivial changes)

  • [x] Clean up commit history

Merged On 28 Sep 2022 at 02:17:26

Cburgdorf

Great stuff!

Commented On 28 Sep 2022 at 02:17:26
Merge

Cburgdorf

Struct pattern

Add struct pattern

Add a struct pattern that can be used in match statements. e.g.,

struct MyS {
    pub x: i32
    pub b: bool

    fn eval(self) -> i32 {
        match self {
            MyS {x: x, b: true} => {
                return x
            }
            MyS {x: x, b: false} => {
                return -x
            }
        }
    }
} 

NOTE

  1. Currently, field labels are required even if a binding name is the same as the field name.
  2. To bind a field, the fields must be pub. Otherwise, the compiler complains about it.

FIx #794

enum Accumulator {
    Zero
    Int(u32)
    Clo(i32, i32)
}

struct MyStruct {
    pub acc: Accumulator
    pub y: i32
}

contract Bar {
} 

I noticed the above example still causes an ICE even in the current master. In #798, I overlooked that we should modify codegen/abi.rs so that non-emittable structs are excluded from abi events. I fixed the problem in this PR for testing a nested struct pattern.

  • [x] OPTIONAL: Update Spec if applicable

  • [x] Add entry to the release notes (may forgo for trivial changes)

  • [x] Clean up commit history

Forked On 28 Sep 2022 at 02:14:49

Cburgdorf

Was this supposed to stay here?
On 28 Sep 2022 at 02:14:49
Merge

Cburgdorf

Struct pattern

Add struct pattern

Add a struct pattern that can be used in match statements. e.g.,

struct MyS {
    pub x: i32
    pub b: bool

    fn eval(self) -> i32 {
        match self {
            MyS {x: x, b: true} => {
                return x
            }
            MyS {x: x, b: false} => {
                return -x
            }
        }
    }
} 

NOTE

  1. Currently, field labels are required even if a binding name is the same as the field name.
  2. To bind a field, the fields must be pub. Otherwise, the compiler complains about it.

FIx #794

enum Accumulator {
    Zero
    Int(u32)
    Clo(i32, i32)
}

struct MyStruct {
    pub acc: Accumulator
    pub y: i32
}

contract Bar {
} 

I noticed the above example still causes an ICE even in the current master. In #798, I overlooked that we should modify codegen/abi.rs so that non-emittable structs are excluded from abi events. I fixed the problem in this PR for testing a nested struct pattern.

  • [x] OPTIONAL: Update Spec if applicable

  • [x] Add entry to the release notes (may forgo for trivial changes)

  • [x] Clean up commit history

Forked On 28 Sep 2022 at 02:12:50

Cburgdorf

Oops :sweat_smile:
On 28 Sep 2022 at 02:12:50

Cburgdorf

Ensure only encodable structs implement emittable

Pushed On 28 Sep 2022 at 09:39:08
Pull Request

Cburgdorf

Ensure only encodable structs implement emittable

Created On 28 Sep 2022 at 09:39:07

Cburgdorf

Ensure only encodable structs implement emittable

Pushed On 28 Sep 2022 at 07:26:33
Pull Request

Cburgdorf

Ensure only encodable structs implement emittable

Created On 28 Sep 2022 at 07:17:39
Create Branch
Cburgdorf In cburgdorf/fe Create Branchchristoph/fix/enum-no-emit

Cburgdorf

A pythonic smart contract language implemented in Rust.

On 28 Sep 2022 at 07:14:24

Cburgdorf

Forbid returning unencodable types from public contract functions

fixes #795

Merged On 26 Sep 2022 at 01:24:34

Cburgdorf

Commented On 26 Sep 2022 at 01:24:34
Issue Comment

Cburgdorf

Panics when a struct contains a enum type

What is wrong?

The code below causes panic

enum Accumulator {
    Zero
    Int(u32)
    Clo(i32, i32)
}

struct MyStruct {
    pub acc: Accumulator
    pub y: i32
}

contract Bar {
} 

Ref: https://github.com/ethereum/fe/pull/772#discussion_r970844998

@cburgdorf Suggestion:

  1. I think we still need #event attributes in addition to #index attributes so a struct can contain enum types.
  2. It'd be better to forbid implementing Emittable to private structs.

Forked On 26 Sep 2022 at 12:56:18

Cburgdorf

If Emittable is automatically implemented for Foo, there is no way to add their custom implementation due to conflicting implementations, right?

Ah, right. Except if we decide that a user provided impl Emittable for X always precedes the automatically provided one. But yeah, I'm not opposed to the #event attribute.

Commented On 26 Sep 2022 at 12:56:18
Issue Comment

Cburgdorf

Panics when a struct contains a enum type

What is wrong?

The code below causes panic

enum Accumulator {
    Zero
    Int(u32)
    Clo(i32, i32)
}

struct MyStruct {
    pub acc: Accumulator
    pub y: i32
}

contract Bar {
} 

Ref: https://github.com/ethereum/fe/pull/772#discussion_r970844998

@cburgdorf Suggestion:

  1. I think we still need #event attributes in addition to #index attributes so a struct can contain enum types.
  2. It'd be better to forbid implementing Emittable to private structs.

Forked On 26 Sep 2022 at 12:00:02

Cburgdorf

I think we still need #event attributes in addition to #index attributes so a struct can contain enum types.

How about we just exclude structs with enum fields for now? What would be the benefit of the #event attribute? I guess putting it on a struct that contains an enum should cause an error, right?

It'd be better to forbid implementing Emittable to private structs.

I'm not opposed but would like to understand the reasoning. Couldn't there be scenarios where I don't want the struct to be public and still use it as an event inside the current module?

Commented On 26 Sep 2022 at 12:00:02

Cburgdorf

Remove event type and emit statement

Pushed On 19 Sep 2022 at 03:06:58

Cburgdorf

Support struct field attributes in parser

Pushed On 19 Sep 2022 at 03:06:58

Cburgdorf

Support events via Emittable trait

Pushed On 19 Sep 2022 at 03:06:58

Cburgdorf

Refactor how generics are resolved

Pushed On 19 Sep 2022 at 03:06:58

Cburgdorf

Update tests

Pushed On 19 Sep 2022 at 03:06:58

Cburgdorf

Get rid of attributes in MIR structs

Pushed On 19 Sep 2022 at 03:06:58
Pull Request

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

Created On 19 Sep 2022 at 03:06:57
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 11:56:19

Cburgdorf

No worries, we can try to work out a solution when we co-work in Berlin next week. And if we can not find a satisfying solution than I can live with the current system based on `event` and `emit` until we have all the ingredients to fully implement something like the `Emittable` trait in Fe itself. That said, I do disagree on the question of complexity. I think having the `event` type and the `emit` keyword with all of its extra checks is more complex than having a trait in the std lib that is automagically implemented by the compiler as a temporary solution. Especially since it is also ensured that no one else can try to implement it manually for the time being.
On 15 Sep 2022 at 11:56:19

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Merged On 15 Sep 2022 at 11:56:20

Cburgdorf

Commented On 15 Sep 2022 at 11:56:20
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 10:02:26

Cburgdorf

Removed them.
On 15 Sep 2022 at 10:02:26

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Merged On 15 Sep 2022 at 10:02:27

Cburgdorf

Commented On 15 Sep 2022 at 10:02:27

Cburgdorf

Get rid of attributes in MIR structs

Pushed On 15 Sep 2022 at 10:02:08
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 09:36:51

Cburgdorf

>Again, I'd like to ask how long this stopgap solution will remain I don't know but I would assume it is still a while out until we can implement the actual event logic in Fe code natively. >And I think the core of the codebase is affected too much But wouldn't you agree that the PR actually lowered the complexity of the code base in general? I mean, look at all the event handling logic that was deleted. More code was deleted than added in this PR. It's actually very little code that was added to enable the struct + trait based event handling.
On 15 Sep 2022 at 09:36:51

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Merged On 15 Sep 2022 at 09:36:52

Cburgdorf

Commented On 15 Sep 2022 at 09:36:52

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Merged On 15 Sep 2022 at 09:32:20

Cburgdorf

Commented On 15 Sep 2022 at 09:32:20
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 09:32:20

Cburgdorf

There is no `ImplId` because there is no `impl` block for it. We can not yet implement the actual event logic in Fe code (in that case, there would be no need for compiler magic at all). The PR doesn't change how events are handled internally, it simply changes how they are triggered.
On 15 Sep 2022 at 09:32:20

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Merged On 15 Sep 2022 at 08:30:52

Cburgdorf

Commented On 15 Sep 2022 at 08:30:52
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 08:30:52

Cburgdorf

I could move these to a place closer to their usage. My reasoning for putting them here is that they are simple strings and I dislike using magic strings inline because it is easy to mistype a string without the compiler noticing (e.g. "inexed" but you can not mistype `INDEXED` without the compiler noticing it.
On 15 Sep 2022 at 08:30:52
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 08:26:34

Cburgdorf

It is, but so is the status quo with the `Event` type and the `lower_event_type` function. After all, this PR doesn't aim to change the internal handling of events it only changes the trigger from being `emit SomeEventType` to being trait based via `Emittable.emit(..)`.
On 15 Sep 2022 at 08:26:34

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Merged On 15 Sep 2022 at 08:26:35

Cburgdorf

Commented On 15 Sep 2022 at 08:26:35
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 08:21:14

Cburgdorf

Ah, ok. :+1: Probably no big deal because we can lookup the analyzer type.
On 15 Sep 2022 at 08:21:14

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Merged On 15 Sep 2022 at 08:21:14

Cburgdorf

Commented On 15 Sep 2022 at 08:21:14
Merge

Cburgdorf

Remove event/emit and introduce ctx.emit that works with the Emittable trait

What was wrong?

As described in #717 we want to remove the event type and the emit keyword and make structs usable as events via ctx.emit<T: Emittable>(val: T)

How was it fixed?

  • Removed event type and emit statement
  • Added support for #indexed attribute on struct fields
  • Introduced Emittable trait which is automatically implemented for all structs
    • the trait is not implementable manually (it therefore depends on a private type)
    • the compiler still implements the logic behind the emit trait function internally (just like before)
  • updated all tests and code examples

Forked On 15 Sep 2022 at 08:20:28

Cburgdorf

>how about receiving `Context` instead of the `OutOfReachMarker` type I thought about that as well but that would mean that people can bypass the `ctx.emit(..)` API and instead call `MyEvent().emit(ctx)`. That may be fine (since they are still depending on the `Context` but I kinda like it more if we force them to go through `ctx.emit(..)`. But I don't have very strong feelings on this.
On 15 Sep 2022 at 08:20:28