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

Extend pkg with delegate/redelegate/unbond and withdraw #12

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MarcelMWS
Copy link

extend pkg

@pasqenr
Copy link
Collaborator

pasqenr commented Nov 2, 2020

I have some concerns about this PR in its current status.

  1. We must wait that @MarcelMWS update his repository with the latest commits merged recently ;
  2. The fields in the new classes use a snake_case naming (e.g. delegator_address) but the Dart naming style suggest the lowerCamelCase coding style for identifiers;
  3. The new classes extend StdMsg and override the getter of the field value, e.g.
@override
Map<String, dynamic> get value => {
  'delegator_address': this.delegator_address,
  'validator_src_address': this.validator_src_address,
  'validator_dst_address': this.validator_dst_address,
  'amount': this.amount.map((coin) => coin.toJson()).toList(),
};

I think that a better way of doing this is to use directly the constructor:

MsgDelegate({
  @required this.delegator_address,
  @required this.validator_address,
  @required this.amount,
  })  : assert(delegator_address != null),
    assert(validator_address != null),
    assert(amount != null),
    super(
      type: "cosmos-sdk/MsgDelegate",
        value: {
          'delegator_address': delegator_address,
          'validator_address': validator_address,
          'amount': amount.map((coin) => coin.toJson()).toList(),
        },
    );

The only issue can be the immutability of the StdMsg field value because currently it's non-final. @marcotradenet maybe we should consider making it final directly in the StdMsg superclass (final Map<String, dynamic> value).

  1. Maybe we should re-format the json files in test_resources directory, VSCode format command should be enough.

@MarcelMWS
Copy link
Author

Hi thanks for the review. I will fix this as soon as possible

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.

2 participants